Add Projects feature and comprehensive test coverage#1
Conversation
…ules - Add Projects tab with CRUD (server/lib/projects.js, router routes, UI tab/modal, store.js helpers) for per-project context scoping - Fix skills vanishing on mode apply: backfill missing skills in states GET/POST, SS.loadFromServer(), SS.applyServerStates() - Add Browse button for repo path in handoff modal and local folder linking in skills install modal - Change .fb button styling from transparent to dull purple for visibility - Add 6 new smoke tests (crypto, security, validation, backup, ranking, mutex) covering previously untested server modules - Extend 3 existing tests (chunker, vectorstore, handoffs) with edge cases: empty/corrupt/oversized inputs, replaceVectors, stale index lifecycle, cosine similarity boundaries, updateHandoff, getHandoff, path traversal, slug collisions
…e platform-aware UNC path
…r timestamp assertions
|
Main blocker: Also worth fixing before merge:
|
|
@codex review |
![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: 15291e04cd
ℹ️ 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 taken = new Set([...readBodyDirSlugs(HANDOFFS_DIR), ...readBodyDirSlugs(ARCHIVE_DIR)]); | ||
| const slug = uniqueSlug(tag || title, taken); | ||
| const slug = uniqueSlug(title, taken); |
There was a problem hiding this comment.
This now derives every new handoff slug from title, which breaks callers that treat thread_tag as the stable identity key. In particular, migrateLegacyHandoff checks getHandoff(entry.slug) before import; because created slugs no longer match that legacy slug, re-running migration on the same source re-imports entries and creates duplicates (..., ...-2, etc.) instead of skipping existing records.
Useful? React with 👍 / 👎.
| if (!Array.isArray(reg.projects)) reg.projects = []; | ||
| reg.projects.push(project); | ||
| writeRegistry(reg); |
There was a problem hiding this comment.
If projects.json contains valid but non-object JSON (for example []), readRegistry() returns that value and this code writes reg.projects onto an Array; JSON.stringify(reg) then serializes back to [] and drops the new project entry. The API still returns success and creates project directories, so a malformed-but-parseable registry causes silent metadata loss and orphaned on-disk project data.
Useful? React with 👍 / 👎.
| branches: [master] | ||
| push: | ||
| branches: [master] |
There was a problem hiding this comment.
These branch filters only trigger on master, so if development happens on main (as in this repo), none of the lint/test jobs run for normal pushes or PRs. That leaves the new checks effectively disabled for day-to-day work and allows regressions to merge without CI coverage.
Useful? React with 👍 / 👎.
| if (!Array.isArray(reg.projects)) return { ok: false, error: 'No projects' }; | ||
| const project = reg.projects.find((p) => p.slug === slug); | ||
| if (!project) return { ok: false, error: 'Project not found' }; | ||
| if (patch?.name) project.name = String(patch.name).trim(); |
There was a problem hiding this comment.
The update path checks patch?.name before trimming, so a whitespace-only string passes the condition and then gets stored as "". This bypasses the non-empty-name rule enforced at creation time and produces nameless projects in the registry/UI, which makes listings and management ambiguous.
Useful? React with 👍 / 👎.
| if (!name) return { ok: false, error: 'name is required' }; | ||
|
|
||
| const reg = readRegistry(); | ||
| const taken = new Set((reg.projects || []).map((p) => p.slug)); |
There was a problem hiding this comment.
This assumes reg.projects is an array and calls .map() directly; if the registry is parseable JSON but has an invalid projects type (for example a string), project creation throws a runtime exception instead of returning a validation error. In the HTTP path this bubbles into a 500, so a single malformed registry file can break all project creation requests.
Useful? React with 👍 / 👎.
| return json(res, result, result.ok ? 200 : 400); | ||
| } | ||
| if (p.startsWith('/api/projects/') && req.method === 'PATCH') { | ||
| const slug = decodeURIComponent(p.slice('/api/projects/'.length)); |
There was a problem hiding this comment.
Decoding the path segment without a try/catch means malformed percent-encoding (for example /api/projects/%E0%A4%A) throws URIError and returns a 500 from the top-level error handler. This is a client input error and should be handled as a 400-style validation failure instead of surfacing as an internal server error.
Useful? React with 👍 / 👎.
…stry, clear _localPath on modal reopen
- ui/projects.js: await AppDialog.confirm({message, confirmText, danger})
so Cancel actually stops deletion (previously the un-awaited Promise
was truthy so deletion always proceeded)
- server/lib/projects.js: delete project directory before removing from
registry; surface directory removal failures as {ok:true, warning:...}
instead of silently swallowing errors
- ui/skills-ingest.js: clear SkillsIngest._localPath when reopening the
connect modal so stale paths can't be submitted
- scripts/projects-smoke.js: add 5 tests for delete ordering,
dir-failure warning, and no-warning on success
|
Addressed all three issues in 0efc382: 1. AppDialog.confirm() missing await — Fixed in ui/projects.js:97-102. Now uses await AppDialog.confirm({message, confirmText, danger}) matching the pattern used everywhere else in the UI. Previously the un-awaited Promise was truthy so deletion always proceeded regardless of Cancel. 2. Directory delete ordering and error surfacing — Fixed in server/lib/projects.js:84-101. The directory is now deleted before removing the project from the registry. If directory removal fails, the response includes {ok: true, warning: Directory removal failed: ...} instead of silently swallowing the error. The project is still removed from the registry (the files are stranded either way), but the caller now gets told about it. Added 5 new tests covering: registry removal on dir failure, warning on failure, no warning on success, and delete ordering verification. 3. Stale _localPath on modal reopen — Fixed in ui/skills-ingest.js:115. Added SkillsIngest._localPath = ''; alongside the existing localPath.value = '' in openConnectModal(), so the stored path is always cleared when the modal opens. This is a UI-only change (no server behavior), so no server-side test is applicable. |
|
Thanks for the follow-up fixes. Two of the three review issues look resolved now:
One issue still remains before I’d merge:
I’d change that flow so directory removal failure returns Checks I ran locally on the updated PR:
|
- server/lib/projects.js: deleteProject now returns {ok:false,error} when
directory removal fails; registry entry only removed on success; missing
directory treated as success (not an error)
- server/lib/projects.js: readRegistry normalizes corrupt JSON (array,
non-object) into a valid {version,projects} shape so createProject and
listProjects never throw on malformed registries
- server/lib/projects.js: updateProject rejects whitespace-only names
instead of storing empty strings
- server/lib/handoffs.js: restore tag-first slug generation
(tag || title) so migration slugs stay stable and re-running
migration doesn't create duplicates
- server/router.js: guard decodeURIComponent in project PATCH/DELETE
routes — malformed percent encoding returns 400 instead of 500
- .github/workflows/ci.yml: add 'main' branch to triggers
- ui/projects.js: await AppDialog.confirm (already fixed)
- ui/skills-ingest.js: clear _localPath on modal reopen (already fixed)
- scripts/projects-smoke.js: tests for readRegistry normalization,
whitespace-only name rejection, delete-on-failure semantics,
decodeURIComponent guard, tag-based slug derivation
- scripts/handoffs-smoke.js: test that slug derives from thread_tag
when present
|
Addressed all 6 review comments in commit 5f61eeb: P1 — Handoff slug (tag-based migration compatibility): Restored \uniqueSlug(tag || title, taken)\ in server/lib/handoffs.js so migration slugs stay stable. Added test verifying \ hread_tag\ is used as slug seed when present. P2 — Normalize registry before use: P1 — CI triggers: Added \main\ branch to both \pull_request\ and \push\ triggers in ci.yml. P2 — Reject whitespace-only names in updateProject: \updateProject\ now trims then validates the name, returning {ok:false, error:\x27name cannot be empty\x27}\ for whitespace-only input. Added test. P2 — Guard reg.projects before .map(): Handled by the readRegistry normalization — P3 — Guard decodeURIComponent in project routes: PATCH and DELETE project routes now wrap \decodeURIComponent\ in try/catch, returning 400 for malformed percent encoding. Added HTTP tests. |
|
Re: \deleteProject\ — Fixed in 5f61eeb. The flow now works as you described:
Added tests for all three cases including verification that the project remains in the registry when deletion fails. |
2 participants
Summary
data/projects/<slug>/directories)loadFromServer/applyServerStates.fbbutton visibility with dull purple styling (--accent-bg/--accent-line/--accent-hi) instead of transparentcrypto.js,security.js,validation.js,backup.js,ranking.js,per-key-mutex.jsreplaceVectors, stale lifecycle, cosine boundaries), handoffs (updateHandoff,getHandoff, path traversal, slug collisions)Test plan
npm run test:crypto— API key encrypt/decrypt, env override, corrupt file recoverynpm run test:security— request validation (loopback/CSRF), write-path protection (system dirs,.ssh/.aws, UNC)npm run test:validation— memory/rules/states validators (all error paths)npm run test:backup— read/write, backup/restore cycle, session log capnpm run test:ranking— tokenize, scoreChunk, scoreSkills, chooseKeepernpm run test:mutex— serial per key, parallel across keys, error isolationnpm run test:projects— 78 tests: library CRUD + slug edge cases + HTTP integrationnpm run check(lint:css, typecheck, lint, format:check) clean