feat(domain): add shared/ids.rs entity identifier newtypes (task T-108) by mpiton · Pull Request #24 · mpiton/forgent

mpiton · 2026-05-05T12:52:54Z

Summary

Introduce five validated newtype identifiers for entity IDs across bounded contexts — TaskId, RunId, WorktreeId (UUID v4) and ProfileId, PrId (i64 row IDs) per ARCHI §4.1. Type-system enforces RunId != TaskId even though both wrap the same primitive, killing the "passed wrong identifier" bug class.

Implements Task #108 of Sprint 1 (foundational domain layer). Blocks T-109 (Task entity) and unblocks T-131 (architecture test for forbidden imports).

Why

Hexagonal architecture + DDD requires validated newtypes to distinguish across contexts. Raw Uuid/i64 allows confusing let task_id: Uuid = run_id; // oops. Newtype wrappers + compile-time type distinction are Rust's idiomatic answer. #[serde(transparent)] keeps JSON wire shape identical ("uuid-string", not {0:"uuid-string"}) so frontend TypeScript bindings remain simple.

Changes

New file src-tauri/src/domain/shared/ids.rs: Five newtypes with declarative macros (uuid_newtype!, i64_newtype!) to avoid duplication. Each derives Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize. UUID types have new() -> Self (v4), from_uuid(), Default (satisfies clippy new_without_default); both types have Display for logging.
CHANGELOG updated under [Unreleased].
19 unit tests (18 after /simplify cleanup): new() v4 emission, from_uuid roundtrip, equality + HashSet, Display, serde transparent roundtrip (UUID → bare "string", i64 → bare 123), Send + Sync bounds, Default consistency.
Architecture enforcement: tests/architecture.rs (T-131) pre-exists and remains green — domain mod.rs already forbids tokio/libsql/reqwest/tauri/git2/keyring/portable_pty/notify, and this file imports only std/serde/uuid (whitelist-permitted).

Testing

cd src-tauri

# Unit tests for the newtype module
cargo test --lib domain::shared::ids

# Full workspace test (architecture test checks forbidden imports)
cargo test --workspace

# Linting + coverage
cargo clippy --workspace --all-targets -- -D warnings
cargo fmt --check
cargo llvm-cov --lib --fail-under-lines 90
cargo deny check

All checks green: 89 tests passed, ids.rs 100% line/region/function coverage (domain target ≥90%).

Related Issues

Closes task T-108 (Sprint 1)
Blocks T-109 (Task model wiring), T-131 (architecture test reinforcement)

Notes for Reviewer

Macro design: two file-local macros (uuid_newtype!, i64_newtype!) expand inline at L56-61. Not exported (no #[macro_export]), so scope is tight. Considered proc macro (overkill for 2 shapes, 5 uses) and trait-based blanket impl (impossible without HKT — each type is distinct, and you cannot blanket-impl Default/Display over arbitrary newtypes).
Field visibility: pub struct TaskId(pub Uuid) exposes the inner field as per T-108 task spec technical-details. Future encapsulation via as_uuid() accessors is deferred (belong to an ADR if strictness is desired).
#[serde(transparent)] rationale: JSON shape stays identical to the inner type — UUID serializes as bare quoted string, i64 as bare integer. This is critical for TypeScript binding simplicity at the IPC boundary; codegen (T-117/T-121) will see type TaskId = string via ts-rs export, not a wrapped object.
/simplify follow-up (commit 80a2d68): hoisted #[must_use] from method-level to struct-level so the lint covers all constructors uniformly (including Default::default()). Dropped a misleading test (distinct_uuid_newtypes_*) whose comment claimed a "compile-error guarantee" but only did runtime checks.

Checklist

Tests added and passing (89/89)
Coverage meets target (100% on ids.rs)
Linting clean (clippy -D warnings, fmt, deny check all pass)
No secrets or debug code
CHANGELOG updated
No manual edits to Cargo.toml/package.json (dependencies pre-added in T-101)
Architecture test still passes (forbidden imports check)
Self-reviewed (initial commit + /simplify follow-up both reviewed)

Summary by CodeRabbit

New Features
- Standardized entity identifiers (Task, Run, Worktree, Profile, PR) with distinct types, constructors, Display/Default, and serde-transparent wire format.
Improvements
- More reliable migrations: baseline schema, atomic apply+history updates, idempotency, and in-memory connections skip migrations; improved FK enforcement for persisted stores.
Tests & Documentation
- Expanded tests for IDs, migrations, and schema behavior; CHANGELOG updated.

Introduce TaskId, RunId, WorktreeId (UUID v4) plus ProfileId, PrId (i64 row IDs) per ARCHI §4.1. Each is `#[serde(transparent)]` so the JSON wire shape stays identical to the inner primitive — TypeScript bindings will see `string`/`number`, not `{0:"…"}`. Two private declarative macros (`uuid_newtype!`, `i64_newtype!`) collapse the duplicate impl blocks at compile-time without runtime cost. Type-system distinction is the whole point: `let _: RunId = TaskId::new()` fails at compile-time with `expected RunId, found TaskId`, killing "passed wrong identifier" bugs across bounded contexts. UUID newtypes ship `new()` (v4), `from_uuid()`, `Default` (clippy `new_without_default`) and Display delegates to the inner UUID/i64 for grep-friendly logs. Domain stays import-pure: only `serde` + `uuid` from the libs already permitted in `domain/mod.rs` — `tests/architecture.rs` still passes. 19 unit tests cover v4 emission, from_uuid roundtrip, equality + Hash, Display matching, serde transparent roundtrip (UUIDs as bare quoted strings, i64s as bare integers), Default v4, Send + Sync, and two new() calls producing distinct UUIDs. Coverage 100% lines / 100% regions / 100% functions on `ids.rs` (domain target ≥90%). Blocks T-109 `Task` model wiring.

Hoist `#[must_use]` from each ctor (`new`, `from_uuid`) onto the struct declaration itself so the lint covers `Default::default()` plus any future fallible constructor uniformly — net `-2` attributes per UUID newtype, identical clippy verdict, stronger contract. Drop `distinct_uuid_newtypes_do_not_compare_equal_by_construction`: its body was a runtime-only `assert_eq!(task.0, run.0)` already covered by the three `*_from_uuid_preserves_value` tests, and the comment claimed a "compile-error guarantee" the test never actually exercised (no `compile_fail` doctest). Trim redundant inline comment in `task_id_serde_roundtrip_is_transparent` that restated what the assertion already says. Coverage `ids.rs` stays at 100% lines / 100% regions / 100% functions; `cargo test --workspace` 90 → 89 (1 redundant test removed); clippy + fmt clean.

coderabbitai · 2026-05-05T12:53:06Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b68e7af8-6fa9-4173-90d9-1e23339a0d70

📥 Commits

Reviewing files that changed from the base of the PR and between 80a2d68 and 82b14cb.

📒 Files selected for processing (1)

CHANGELOG.md

🚧 Files skipped from review as they are similar to previous changes (1)

CHANGELOG.md

📝 Walkthrough

Walkthrough

Adds five new entity identifier newtypes (TaskId, RunId, WorktreeId, ProfileId, PrId) implemented via macros in ids.rs. UUID-backed types provide new()/from_uuid() and Default, all types are #[serde(transparent)], include Display impls, unit tests, and a CHANGELOG entry.

Changes

Entity Identifier Types

Layer / File(s)	Summary
Macro Infrastructure `src-tauri/src/domain/shared/ids.rs`	Adds `uuid_newtype!` and `i64_newtype!` macros that generate public newtype structs with derives, `#[serde(transparent)]`, constructors (`new`, `from_uuid` for UUIDs), `Default` (UUIDs), and `Display`.
Type Definitions `src-tauri/src/domain/shared/ids.rs`	Defines five public ID types: `TaskId`, `RunId`, `WorktreeId` (UUID-backed) and `ProfileId`, `PrId` (i64-backed).
Tests & Validation `src-tauri/src/domain/shared/ids.rs`	Adds unit tests covering UUID v4 generation, equality/hash, `Display`, serde round-trips, `Send`/`Sync` bounds, uniqueness, and default behavior.
Documentation `CHANGELOG.md`	Documents the new ID types, follow-up fixes (hoisted `#[must_use]`, removed redundant equality test), and references to migration/pool behavior changes (as descriptive entries).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

feat(infra): apply V001 migration via refinery embed + libsql bridge (task T-107) #23: Referenced in CHANGELOG for migration atomicity and in-memory migration behavior.

Poem

🐰 I stitched five names from bytes and bits,

Uuids hopping, integers sit.
Macros hummed a tidy tune,
Tests and docs beneath the moon.
Small newtypes, big-safe wits.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat(domain): add shared/ids.rs entity identifier newtypes (task T-108)' clearly and specifically summarizes the main change—adding new newtype entity identifier definitions in ids.rs, which aligns directly with the changeset that introduces TaskId, RunId, WorktreeId, ProfileId, and PrId.
Docstring Coverage	✅ Passed	Docstring coverage is 95.24% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/task-T-108-shared-ids

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Line 15: Within the [Unreleased] block there is a duplicate "### Changed"
heading; remove the extra "### Changed" header and merge its bullet points under
the existing "### Changed" section so there's only one such heading in that
release block (also check for and consolidate any duplicate "### Added" headings
similarly).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c6ba5501-50f2-425c-a3be-3a68e889051b

📥 Commits

Reviewing files that changed from the base of the PR and between 1201f79 and 80a2d68.

📒 Files selected for processing (2)

CHANGELOG.md
src-tauri/src/domain/shared/ids.rs

codspeed-hq · 2026-05-05T12:55:29Z

Merging this PR will not alter performance

✅ 7 untouched benchmarks

_{Comparing feat/task-T-108-shared-ids (82b14cb) with main (1201f79)}

cubic-dev-ai

No issues found across 2 files

…sed] blocks Address CodeRabbit MD024 finding on PR #24: the two new T-108 headings (`### Changed` for the /simplify follow-up, `### Added` for the feat commit) were stacked above the pre-existing PR #23 / T-107 blocks, creating two same-name headings inside the same `[Unreleased]` section. Move both T-108 bullets to the top of their respective existing blocks instead so the new content no longer adds duplicate headings. Pre-existing duplicate headings further down the section (`### Changed` at the old T-105/T-104 simplify block, `### Added` at the older T-104/ T-103 block) were already in `main` before this branch and are out of scope for this PR.

mpiton added 2 commits May 5, 2026 14:44

coderabbitai Bot reviewed May 5, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

cubic-dev-ai Bot reviewed May 5, 2026

View reviewed changes

mpiton merged commit a5e1afd into main May 5, 2026
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(domain): add shared/ids.rs entity identifier newtypes (task T-108)#24

feat(domain): add shared/ids.rs entity identifier newtypes (task T-108)#24
mpiton merged 3 commits into
mainfrom
feat/task-T-108-shared-ids

mpiton commented May 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 5, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codspeed-hq Bot commented May 5, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mpiton commented May 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Changes

Testing

Related Issues

Notes for Reviewer

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codspeed-hq Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mpiton commented May 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 5, 2026 •

edited

Loading

codspeed-hq Bot commented May 5, 2026 •

edited

Loading