feat(bridge): workspace-backed project registration + adapter improvements

[serrrfirat]

Summary

• Workspace-backed project registration with commitment migration into projects/commitments/
• LLM adapter routing improvements for model selection and chat history handling
• Store adapter persistence enhancements
• Core routing logic updates for effect/message/tool dispatch
• Added MissionManager::store() accessor for project auto-registration
• Added sync_v1_skill_to_store for live skill install sync to v2 engine

Cherry-picked from #2504.

Test plan

• cargo check passes
• cargo clippy --all --all-features clean
• All 33 effect_adapter unit tests pass (including new project slug extraction and deterministic project ID tests)
• All 3 skill_migration tests pass
• All 402 ironclaw_engine tests pass
• Verify project registration persists across restarts
• Confirm LLM adapter correctly routes to configured model

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request transitions the engine to a project-centric workspace model where projects are implicitly defined by their directory structure under projects/. Key updates include deterministic project ID generation using UUID v5, auto-registration of projects during memory writes, and a migration of all persona skills to the new pathing conventions. The bridge layer was also enhanced to sync runtime skill installations to the v2 store and resolve project references by slug or name. Feedback identifies a potential ambiguity in project resolution due to prefix-based slug matching and suggests adopting more idiomatic Rust string trimming methods in the slugification utility.

[gemini-code-assist]

The slug matching logic name_slug.starts_with(&format!("{needle}-")) allows for prefix matching of slugs. This could lead to ambiguity if multiple projects have slugs with the same prefix (e.g., my-project-a and my-project-b would both match my-project). Since find() returns the first match, this could result in selecting the wrong project. Use exact matching or ensure proper word boundaries are checked to avoid false positives.

References

1. When detecting commands or keywords in a string, use token-based or word-boundary checks instead of simple substring containment to avoid false positives.

[gemini-code-assist]

This loop for removing trailing dashes can be made more idiomatic and potentially more efficient by using trim_end_matches and truncate to handle character boundaries correctly and improve readability.

    let new_len = out.trim_end_matches('-').len();
    out.truncate(new_len);

References

1. Prefer idiomatic string trimming methods like trim_matches or trim_end_matches over manual loops or chained prefix/suffix stripping for better readability and performance.

[henrypark133]

1. High – src/bridge/effect_adapter.rs:1302-1308 now resolves a project reference by accepting any slug whose normalized name merely starts with "{needle}-". That makes references ambiguous as soon as one project slug is a prefix of another, so a write intended for one project can be rebound to a different project owned by the same user. This path is used to resolve project-backed memory writes, so the failure mode is silent misrouting rather than a clean "multiple matches" error.

I would keep the exact-name / exact-slug matches, but drop the prefix fallback unless the API also detects and rejects ambiguous matches explicitly.

Residual risk: the workspace-backed registration flow is broad, so I also spot-checked the synthetic-project path and migration adapter behavior. I did not find a second verified blocker there.

[zmanian]

Summary

Large PR (+1041/-194 across 21 files) that makes projects/<slug>/ the user-facing project layout with deterministic ProjectId::from_slug(user_id, slug) via UUID v5, auto-registers a Project entity on memory_write to projects/<slug>/..., syncs live-installed v1 skills into the v2 engine doc store, and renames several persona skills (ceo-assistant → ceo-setup, etc.) with setup_marker-gated activation. Core design is sound; concerns are mostly around slug resolution ambiguity, a breaking skill rename, and scope creep.

Findings

Blocking

None, but the slug-prefix matching and skill rename deserve explicit sign-off before merge.

Suggestions

• Slug-prefix resolution is ambiguous (src/bridge/effect_adapter.rs:1307-1322, resolve_project_ref). The slug-matching branch uses name_slug.starts_with(&format!("{needle}-")). If a user has both commit and commitments projects, mission_create(project_id: "commit") matches commit (exact) — fine. But mission_create(project_id: "comm") matches commitments first iff it sorts first in list_projects, and ordering is storage-backend-dependent. Prefer exact-match-only with a fallback to "ambiguous prefix — please specify" when multiple projects match. Resolving silently to the wrong project is a correctness footgun.
• Bare-project visibility leak (src/bridge/store_adapter.rs:674-709, synth_bare_project). A projects/<slug>/ directory without .project.json gets synthesized with user_id = shared_owner_id(). Then list_projects(real_user_id) won't return it (filters by user_id). Two issues: (a) the user who owns the workspace can't see their own bare project in the dashboard; (b) the bare project leaks into the shared-owner namespace, so it becomes globally visible. If the workspace always flows through ensure_project_for_memory_write (which correctly uses the real user_id), bare projects only arise from the migration path. Worth either (i) deriving user_id from the workspace owner when synthesizing, or (ii) documenting that bare projects are a migration/cleanup artifact and filtering them out of list_projects unless the caller opts in.
• Path traversal on projects/<slug>/... (src/bridge/effect_adapter.rs:1252-1269, extract_project_slug_from_target). Rejects ., .., .hidden correctly. But this runs AFTER a successful memory_write, so the path has already been resolved by the workspace layer. If memory_write ever passes through URL-encoded or Unicode-normalized variants (projects/%2e%2e/evil/foo.md, projects/⋯/evil.md), the slug extractor here sees the pre-resolved form. Verify the workspace layer canonicalizes first — if not, we have a project-namespace confusion vector. Low probability but worth pinning with a test.
• Breaking skill rename without migration (skills/ceo-assistant/SKILL.md → ceo-setup, content-creator-assistant → content-creator-setup, developer-assistant → developer-setup). Users or workspaces that reference the old skill names (via /ceo-assistant slash commands, saved routines, scheduled jobs, MEMORY.md pointers) get silently broken. The setup_marker activation gate assumes clean installs. Either (a) leave compatibility shims for the old names that re-point, or (b) document the migration explicitly and add a startup check that warns if the old names are referenced anywhere. This is the item most likely to surprise users post-merge.
• sync_v1_skill_to_store uses shared_owner_id() (src/bridge/skill_migration.rs:130). Intentional per comment ("registry-sourced"), but means every user sees the installed skill as a shared doc in their project. If an attacker installs a skill via skill_install from user A's session, user B now has it in their v2 doc space too. For trusted-registry skills this is fine; for arbitrary names-as-keys across users this could be surprising. Confirm skill_install approval gating prevents cross-user pollution before merge.
• Scope creep. This PR bundles: project storage migration + skill v2 sync + 5 persona skill rewrites + MissionManager::store() accessor + UUIDv5 feature flag. Any one of these is a PR on its own. Future cherry-picks from #2504 would land better as smaller slices.
• Migration idempotence corner case (src/bridge/store_adapter.rs:663-707, migrate_legacy_project_jsons). If ws.write(new_path) succeeds but ws.delete(legacy_path) silently fails (let _ = ws.delete(...)), next startup sees both files. The code guards with ws.read(&new_path).await.is_ok() and tries the delete again — correct — but a persistent delete failure leaves the legacy path forever. Consider logging debug! on the delete failure so you notice if this gets stuck in prod.
• Dual-backend compliance. All new persistence goes through the Store trait (list_projects, save_project, save_memory_doc, list_shared_memory_docs) — no new raw SQL, so Postgres and libSQL both work. Good.

Nits

• crates/ironclaw_engine/src/types/project.rs:77 — the PROJECT_ID_NAMESPACE UUID is load-bearing ("burning this value would rotate every user's project IDs"). Worth a #[test] fn namespace_is_stable that asserts the UUID hasn't changed and Project::new("user-1", "commitments", "") produces a pinned UUID. CI guard against accidental edits.
• src/bridge/effect_adapter.rs:1256 — slug extractor doesn't explicitly reject trailing whitespace or path separators inside the slug. slugify_simple would collapse them, but the direct rest.split_once('/') split doesn't. A target like projects/ foo /bar.md yields slug " foo ". Likely rejected downstream, but worth a test.
• crates/ironclaw_engine/Cargo.toml:29 — adding v5 to the uuid crate features is the right minimal change.

Verdict

COMMENT — core design (workspace as project source of truth, deterministic IDs, auto-registration) is right. The slug-prefix ambiguity and skill-rename breaking change are the two items I'd resolve (or explicitly accept) before merge. Everything else is incremental hardening and scope-reduction feedback for future PRs.

[zmanian]

Cross-cutting concern (across #2530, #2531, this PR)

Not specific to this PR — posting here because it's the freshest example. Flagging a pattern that emerged while reviewing the six-PR batch cherry-picked from #2504.

Three of the six PRs add a "convenience shortcut" that moves a security-relevant decision from an explicit approval gate to a downstream predicate:

1. feat(skills): activation feedback pipeline + install idempotence #2530 — skill_install skips approval when the named skill is already loaded. Relies on execute() being a no-op for the companion-install path when install_dependencies=true. If that contract ever shifts, approval is silently bypassed.
2. fix(engine): FINAL-await support + runaway loop protection #2531 — /<skill-name> slash mentions force-activate a skill regardless of score. The gate is now "skill is present in the attenuated list" rather than "skill scored above threshold + budget permits". A prompt-injection payload can invoke any skill already in the user's set without passing the keyword/tag scoring that was designed to require intent.
3. feat(bridge): workspace-backed project registration + adapter improvements #2533 — resolve_project_ref matches on slug prefix (name_slug.starts_with(&format!("{needle}-"))). When multiple projects share a prefix, ordering is storage-dependent and the wrong project can be silently selected for a mutation.

None of these is catastrophic in isolation, and each has a sensible UX motivation (don't nag about an already-loaded skill, let the user name skills directly, tolerate abbreviated project names). Individually I approved each with a callout.

The pattern worth discussing: the safety properties of the skill/project/tool system are increasingly "gated by a predicate downstream" rather than "gated by approval at the use site". That works as long as every downstream predicate stays airtight. It scales poorly — each new convenience shortcut adds an implicit contract that the next refactor has to preserve, and you can't grep for an implicit contract.

Two questions worth a short async thread before this batch lands:

• Is there a principle we want to write down? For example: convenience shortcuts that skip approval must be tested at the caller level with a test that asserts the dangerous action doesn't fire, not just that approval is skipped. That would cover feat(skills): activation feedback pipeline + install idempotence #2530's install_dependencies=true path directly.
• For fix(engine): FINAL-await support + runaway loop protection #2531 and feat(bridge): workspace-backed project registration + adapter improvements #2533 specifically, would exact-match-only (no slug prefix, no slash force-select without score floor) be cheap enough UX-wise that we'd prefer it? The failure mode for both is "silent wrong action"; the failure mode for exact-match is "user re-types the full name" — strictly noisier but strictly safer.

Happy to split this into a separate design issue if you'd rather not thread on a PR. Pinging here because the reviewers of all three PRs (and the author of #2504) are already watching.

cc @serrrfirat

[henrypark133]

Review: project reference resolution is still ambiguous

The workspace-backed registration work is useful, but the slug-resolution blocker is still present in the current head.

Concerning: prefix fallback can still bind a write to the wrong project

File: src/bridge/effect_adapter.rs:1302
resolve_project_ref() still accepts name_slug.starts_with(&format!("{needle}-")) as a match. Once one project slug is a prefix of another, a partial reference can silently resolve to whichever project happens to appear first in list_projects(...). This path is used to resolve project-backed memory and mission actions, so the failure mode is silent misrouting rather than a clean ambiguity error.

Suggested fix: keep exact UUID / exact name / exact slug matches, but drop the prefix fallback unless the API also detects and rejects multiple matches explicitly.

Summary:

• Recommended verdict: Request changes
• Prior feedback status: partially unresolved; the slug-prefix ambiguity remains in the current head
• Residual risk: I also spot-checked the bare-project synthesis path and it still carries shared-owner fallback semantics worth revisiting, but the prefix match is the concrete blocker.

[ilblackdragon]

Merged origin/staging (d2c040c) — ~80 PRs worth of divergence resolved. Review follow-ups also applied:

Addressed in this push

• @henrypark133 / @zmanian — slug-prefix resolution ambiguity (resolve_project_ref) — fixed in c88ad2a before this merge. Only exact UUID / exact name / exact slug matches remain; the name_slug.starts_with(format!("{needle}-")) fallback is gone.
• @zmanian — bare-project visibility leak (synth_bare_project) — synth_bare_project now takes the workspace owner's user_id (passed from ws.user_id() in load_projects_from_workspace) instead of shared_owner_id(). Bare projects/<slug>/ dirs now surface in list_projects(real_user_id) and don't leak into the shared-owner namespace.
• @zmanian — PROJECT_ID_NAMESPACE stability nit — namespace_uuid_is_stable now also pins the derived UUID for Project::new("user-1", "commitments", "") (aa38ce02-4359-5fa1-9d8b-efa8b573a353), so any drift in ProjectId::from_slug (namespace or seed format) fails CI instead of silently re-ID'ing every workspace-backed project.
• @zmanian — sync_v1_skill_to_store cross-user pollution — merged to staging's list_skills_global() path, which deduplicates shared skill docs across projects. Confirmed existing shared skills get updated in place rather than duplicated per project. The merge also pulled in staging's bundle_path / source_url fields on V2SkillMetadata.

Intentionally not changed

• @zmanian — path-traversal defense-in-depth on extract_project_slug_from_target — slug_extractor_whitespace_and_special test is already in c88ad2a, and the helper rejects ., .., and dotfiles explicitly. The Workspace layer canonicalizes before the helper runs (verified via ws.write → path resolution → memory_write output → ensure_project_for_memory_write); adding a second canonicalization here would be duplicate defense. Happy to revisit if someone can demonstrate a bypass.
• @zmanian — breaking skill rename — the rename from *-assistant → *-setup already landed on staging via a separate PR (feat(skills): setup-marker lifecycle, chain-loading, and live GitHub workflow test #2268 skill-lifecycle work); this merge just reconciles marker paths. No compat shim needed since the rename predates this PR.
• @zmanian — migration idempotence ws.delete silent failure — migrate_legacy_project_jsons already logs debug! on delete failure at both call sites (lines 708-710, 721-723). No change needed.
• @zmanian — cross-cutting "gated by predicate downstream" pattern concern — agree this deserves a dedicated discussion; the prefix-fallback removal here at least narrows that particular surface. Worth a separate design issue rather than gating this PR.

Verification

• cargo check --all-features ✅
• cargo clippy --all --tests --all-features -- -D warnings ✅ (clean)
• cargo test --lib -p ironclaw_engine — 437 passed
• cargo test --lib bridge:: — 298 passed

tools::builtin::skill_tools::tests::test_zip_* failures reproduce on pristine origin/staging and are unrelated to this PR.

🤖 Generated with Claude Code

addressed

[ilblackdragon]

Review — workspace-backed project registration + adapter improvements

Summary: Promotes projects/<slug>/ from an engine-internal path to the user-facing declaration gesture, adds deterministic ProjectId::from_slug (UUID v5), and auto-registers projects on memory_write. Also extracts resolve_project_ref for mission_create and adds live-install sync for skills.

What's good

• ProjectId::from_slug + pinned PROJECT_ID_NAMESPACE test (project.rs:119-125) is the right shape — idempotency is enforced at the ID layer, so duplicate save_project calls under concurrent writes can't fork the project entity.
• Extracting resolve_project_ref out of the inline mission_create block is a clean win; de-dupes the slug-resolution with the existing UUID validator.
• extract_project_slug_from_target rejects ., .., and .hidden prefixes — path traversal is blocked.
• IDOR: ensure_project_for_memory_write scopes list_projects(user_id) to context; cannot synthesize cross-user projects.

Correctness issues

1. Inconsistent project ID derivation between the two creation paths. (likely bug)

• `ensure_project_for_memory_write` (effect_adapter.rs:1261) constructs via `Project::new(user_id, slug, "")`, which internally does `slugify_simple(name)` before `ProjectId::from_slug`.
• `synth_bare_project` (store_adapter.rs:1755-1769) constructs directly with `ProjectId::from_slug(user_id, slug)` using the raw directory name as slug, bypassing `slugify_simple`.

For a path like `projects/My Project/foo.md` (or any slug that `slugify_simple` would normalize), the two code paths produce different `ProjectId`s for the same directory. Workspace reload via `load_projects_from_workspace` sees ID A; the next `memory_write` creates ID B. A duplicate project entity materializes.

The common `projects/commitments/...` case works because `slugify_simple("commitments") == "commitments"`. A non-canonical slug is the trigger.

Fix: have `synth_bare_project` call `ProjectId::from_slug(user_id, &slugify_simple(slug))` (and store `name: slugify_simple(slug)`), or add a `Project::from_workspace_dir(user_id, raw_slug)` helper so both paths go through the same normalization. Add a regression test that drives a non-canonical slug through both paths and asserts the IDs match.

2. `project_slug` on `HybridStore` (store_adapter.rs:830-840) still uses the old `slugify(name, uuid)` with UUID suffix, while `project_dir`/`project_path` now use `project_slug_for_name` (no UUID). Confirm there's no call site that combines the two — e.g. a mission written to `{project_slug}/missions/...` using the old scheme while the project metadata lives at `projects/{new-slug}/.project.json`. If `project_slug` still targets `.system/engine/projects/...` for missions only, leave a comment on the function clarifying scope; otherwise you'll get silently orphaned missions on rename.

3. Migration failure mode silently drops projects. `migrate_legacy_project_jsons` logs at `debug!` when parse/write/delete fails (store_adapter.rs:1714, 1727, 1733). A user whose legacy `project.json` fails to migrate loses their project metadata with no visible warning. Upgrade to `warn!` and — per CLAUDE.md — ensure the migration is retried on the next load (it already is, because the legacy file is only deleted after successful write — good). But a parse failure (line 1714) never retries. Consider moving the bad file aside (`.broken.json`) rather than leaving it to re-fail silently on every boot.

4. Race on auto-register. `ensure_project_for_memory_write` does list→find→create with no lock. Two concurrent `memory_write`s to the same project slug both miss the existing project. Because IDs are deterministic, `save_project` becomes a repeat upsert with the same ID — tolerable, but document this. If `save_project` clobbers `created_at`, that's a real issue. Worth confirming in the `Store` impl.

Testing gaps (per `.claude/rules/testing.md` — "Test Through the Caller, Not Just the Helper")

`extract_project_slug_from_target` is a predicate that gates a side effect (`save_project`) with a wrapper (`ensure_project_for_memory_write`) between them, and the wrapper computes additional inputs (user_id from context). All three conditions from the rule apply:

• Add an integration-tier test that drives `execute_action("memory_write", ...)` with `target = "projects/newproj/AGENTS.md"` and asserts (a) the output includes `project_id`, (b) a project named `newproj` exists in the store afterward, (c) a second write to the same slug returns the same `project_id` and does not create a duplicate.
• Add a test for the slug-mismatch bug in (1): write to `projects/ foo /bar.md` (or any slug that `slugify_simple` normalizes), reload the store, and assert the in-memory project ID equals a subsequent `Project::new` with the same inputs.

Style nits

• `project_dir(name, _project_id)` (store_adapter.rs:1796) takes an unused `ProjectId`. Either drop the param and update call sites or keep it typed for future use with a comment — leaving `_project_id` unused is drift bait.
• The `Project::new` docstring (project.rs:81-91) runs eight lines; CLAUDE.md pushes toward one-line doc comments. Not a blocker, but worth trimming to the invariant ("ID is deterministic from `(user_id, slug(name))`; use `ProjectId::new()` for throwaway fixtures").
• `slugify_simple` is in `types/mod.rs` but not re-exported at the crate root. `effect_adapter` accesses it via `ironclaw_engine::types::slugify_simple` — fine, but inconsistent with `Project` / `ProjectId` which are at the crate root. Either bring both to the same level or leave a comment.

Security

No new concerns beyond what's above. Path traversal, IDOR, and auth surfaces are all properly gated.

Verdict

Solid direction — workspace-as-declaration is a clean abstraction. Blocking issues are #1 (ID mismatch between `synth_bare_project` and `Project::new`) and the missing caller-level test that would have caught it. The migration fallback behavior and `project_slug` scoping are worth clarifying before merge.

[ilblackdragon]

Pushed 9838c01 addressing the review. Quick walk-through:

1. Slug mismatch between synth_bare_project and Project::new — fixed. synth_bare_project now normalizes the raw dir name via slugify_simple before ProjectId::from_slug and returns Option<Project>, rejecting dirs like --- or !!! that have no canonical form. The two construction paths now produce identical IDs for every input shape.

2. project_slug vs project_dir scope — added a comment on project_slug documenting that it's the engine-internal UUID-suffixed mission-path scheme, disjoint from the user-facing project_dir (no-UUID). No call sites mix the two; missions stay under .system/engine/projects/… and user metadata lives at projects/<slug>/.project.json.

3. Migration silent failure mode — upgraded debug! → warn! throughout migrate_legacy_project_jsons. For unparseable JSON, the bad file is now moved aside to project.broken.json so it doesn't re-fail on every boot and the user can recover it manually.

4. Auto-register race — deterministic ProjectId::from_slug means duplicate save_project is a same-ID upsert. Added a caller-level test (memory_write_is_idempotent_across_repeated_writes) that drives three sequential writes to the same project and asserts exactly one project results. save_project in HybridStore preserves the insert's created_at on upsert since the passed Project keeps its original timestamp — no clobber.

Style nits — also addressed:

• Dropped the unused _project_id param from project_dir / project_path.
• Trimmed Project::new docstring to one line per CLAUDE.md.
• Left slugify_simple at types/mod.rs with the path-qualified access — re-export parity was a toss-up and the explicit path call documents the layer.

Tests added (19) — per .claude/rules/testing.md "Test Through the Caller":

• types::project (4): slug variants collapse, unicode, empty-slug stability, run/edge normalization
• store_adapter unit (6): project_slug_for_name, project_dir/project_path, synth_bare_project↔Project::new ID equivalence over 12 weird names, unsluggable-dir rejection, cross-user isolation
• store_adapter libsql migration tests (6): bare-dir load, metadata-over-synth, non-canonical skip, weird-slug collapse, user-edit preservation, broken-JSON move-aside
• effect_adapter caller-level (7): drives execute_action("memory_write") through the full hook for canonical / idempotent / non-projects/ / nested / weird-slug / cross-user / pathological cases

cargo fmt, cargo clippy --all --benches --tests --examples --all-features (zero warnings), and all 19 new tests pass.