docs: add refactoring implementation plan
This commit is contained in:
@@ -17,3 +17,4 @@ apps/ui/test-results
|
|||||||
apps/ui/vite.config.js
|
apps/ui/vite.config.js
|
||||||
apps/ui/vite.config.d.ts
|
apps/ui/vite.config.d.ts
|
||||||
*.log
|
*.log
|
||||||
|
__*.md
|
||||||
|
|||||||
@@ -2,21 +2,27 @@
|
|||||||
|
|
||||||
## Current
|
## Current
|
||||||
|
|
||||||
### `feat/runtime-env-simplification`
|
### `feat/refactoring-plan`
|
||||||
|
|
||||||
Status: in_progress
|
Status: in_progress
|
||||||
|
|
||||||
DoD:
|
DoD:
|
||||||
- runtime uses component-based PostgreSQL config as the primary model
|
- `__REFACTORING.md` documents the full remediation plan for registry modularization, sqlx compile-time validation, KDF correction, typed timestamps, ID display support, pool tuning, and error normalization
|
||||||
- `CRANK_BASE_URL` replaces split public URL variables
|
- `.gitignore` ignores local `__*.md` working notes and refactoring specs
|
||||||
- deployment uses one GitHub secret per runtime env variable
|
- `TASKS.md` points to the refactoring track as the active work item
|
||||||
- deprecated env variables are removed from compose, examples, and docs
|
|
||||||
|
|
||||||
## Next
|
## Next
|
||||||
|
|
||||||
- `feat/websocket-test-run-polish`
|
- `feat/postgres-registry-modularization`
|
||||||
|
|
||||||
## Backlog
|
## Backlog
|
||||||
|
|
||||||
|
- `feat/postgres-registry-modularization`
|
||||||
|
- `feat/sqlx-compile-time-verification`
|
||||||
|
- `feat/typed-timestamps`
|
||||||
|
- `feat/secret-crypto-hkdf`
|
||||||
|
- `feat/id-display-support`
|
||||||
|
- `feat/postgres-pool-config`
|
||||||
|
- `feat/error-structure-normalization`
|
||||||
- `feat/websocket-test-run-polish`
|
- `feat/websocket-test-run-polish`
|
||||||
- `feat/live-authenticated-staging-entry`
|
- `feat/live-authenticated-staging-entry`
|
||||||
|
|||||||
@@ -0,0 +1,932 @@
|
|||||||
|
# Refactoring Plan
|
||||||
|
|
||||||
|
## Purpose
|
||||||
|
|
||||||
|
This document defines the refactoring track required to improve maintainability, safety, and production-readiness in the current codebase. It is intentionally implementation-oriented: each area is split into concrete slices, target modules, function ownership, migration order, and acceptance criteria.
|
||||||
|
|
||||||
|
The refactoring scope covers:
|
||||||
|
|
||||||
|
1. splitting `crates/crank-registry/src/postgres.rs` into domain modules;
|
||||||
|
2. migrating static SQL to `sqlx::query!` / `sqlx::query_as!` where possible;
|
||||||
|
3. replacing SHA-256 key derivation in `SecretCrypto` with HKDF;
|
||||||
|
4. replacing string timestamps in domain models with typed timestamps;
|
||||||
|
5. adding `Display` support to `define_id!`;
|
||||||
|
6. introducing explicit PostgreSQL pool configuration;
|
||||||
|
7. removing duplicated SQL timestamp formatting;
|
||||||
|
8. making runtime and API error payloads more structured.
|
||||||
|
|
||||||
|
## Guiding Rules
|
||||||
|
|
||||||
|
- Keep domain logic separate from SQL storage concerns.
|
||||||
|
- Preserve current behavior while shrinking file size and coupling.
|
||||||
|
- Move in vertical slices: storage module split first, then SQL typing, then timestamp migration.
|
||||||
|
- Every slice must be independently reviewable and mergeable.
|
||||||
|
- Prefer compatibility shims during migration instead of breaking multiple crates at once.
|
||||||
|
- Add or update tests in the same slice as the refactoring.
|
||||||
|
|
||||||
|
## Refactoring Tracks
|
||||||
|
|
||||||
|
### Track A: Registry Modularization
|
||||||
|
|
||||||
|
#### Problem
|
||||||
|
|
||||||
|
`crates/crank-registry/src/postgres.rs` currently concentrates almost all database logic:
|
||||||
|
|
||||||
|
- bootstrap and auth;
|
||||||
|
- workspaces and memberships;
|
||||||
|
- invitations;
|
||||||
|
- operations and versions;
|
||||||
|
- agents and bindings;
|
||||||
|
- samples and descriptors;
|
||||||
|
- API keys;
|
||||||
|
- secrets;
|
||||||
|
- stream sessions and async jobs;
|
||||||
|
- invocation logs and usage;
|
||||||
|
- YAML import jobs.
|
||||||
|
|
||||||
|
This creates:
|
||||||
|
|
||||||
|
- a permanent git conflict hotspot;
|
||||||
|
- slow code navigation;
|
||||||
|
- oversized review diffs;
|
||||||
|
- accidental coupling between unrelated domains.
|
||||||
|
|
||||||
|
#### Target Layout
|
||||||
|
|
||||||
|
`crates/crank-registry/src/`
|
||||||
|
|
||||||
|
```text
|
||||||
|
error.rs
|
||||||
|
lib.rs
|
||||||
|
migrations.rs
|
||||||
|
model.rs
|
||||||
|
postgres/
|
||||||
|
mod.rs
|
||||||
|
connect.rs
|
||||||
|
auth.rs
|
||||||
|
workspace.rs
|
||||||
|
invitation.rs
|
||||||
|
secret.rs
|
||||||
|
stream.rs
|
||||||
|
api_key.rs
|
||||||
|
operation.rs
|
||||||
|
agent.rs
|
||||||
|
observability.rs
|
||||||
|
yaml_import.rs
|
||||||
|
shared.rs
|
||||||
|
```
|
||||||
|
|
||||||
|
#### Ownership by Module
|
||||||
|
|
||||||
|
##### `postgres/mod.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- define `PostgresRegistry`;
|
||||||
|
- wire submodules;
|
||||||
|
- expose the public impl surface;
|
||||||
|
- keep only high-level constructor entry points.
|
||||||
|
|
||||||
|
Functions staying here:
|
||||||
|
|
||||||
|
- `new`
|
||||||
|
- `connect`
|
||||||
|
- `connect_with_options`
|
||||||
|
- `migrate`
|
||||||
|
|
||||||
|
Functions moving out:
|
||||||
|
|
||||||
|
- everything domain-specific.
|
||||||
|
|
||||||
|
##### `postgres/connect.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- connection setup;
|
||||||
|
- pool initialization;
|
||||||
|
- migration invocation;
|
||||||
|
- pool option translation from config to `PgPoolOptions`.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `connect`
|
||||||
|
- `connect_with_options`
|
||||||
|
- `migrate`
|
||||||
|
- internal helpers:
|
||||||
|
- `build_pool_options`
|
||||||
|
- `build_connect_options`
|
||||||
|
|
||||||
|
##### `postgres/auth.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- auth users;
|
||||||
|
- user sessions;
|
||||||
|
- membership access checks;
|
||||||
|
- auth profiles.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `upsert_bootstrap_user`
|
||||||
|
- `get_auth_user_by_email`
|
||||||
|
- `get_auth_user_by_id`
|
||||||
|
- `update_user_profile`
|
||||||
|
- `update_user_password`
|
||||||
|
- `create_user_session`
|
||||||
|
- `get_user_session`
|
||||||
|
- `touch_user_session`
|
||||||
|
- `revoke_user_session`
|
||||||
|
- `set_user_session_current_workspace`
|
||||||
|
- `user_has_workspace_access`
|
||||||
|
- `save_auth_profile`
|
||||||
|
- `get_auth_profile`
|
||||||
|
- `list_auth_profiles`
|
||||||
|
- `list_auth_profiles_referencing_secret`
|
||||||
|
|
||||||
|
Internal helpers to extract:
|
||||||
|
|
||||||
|
- `map_auth_user_row`
|
||||||
|
- `map_session_row`
|
||||||
|
- `map_auth_profile_row`
|
||||||
|
|
||||||
|
##### `postgres/workspace.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- workspaces;
|
||||||
|
- memberships;
|
||||||
|
- workspace CRUD.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `list_workspaces`
|
||||||
|
- `list_workspaces_for_user`
|
||||||
|
- `ensure_membership`
|
||||||
|
- `list_memberships`
|
||||||
|
- `update_membership_role`
|
||||||
|
- `delete_membership`
|
||||||
|
- `get_workspace`
|
||||||
|
- `create_workspace`
|
||||||
|
- `update_workspace`
|
||||||
|
- `delete_workspace`
|
||||||
|
|
||||||
|
Internal helpers:
|
||||||
|
|
||||||
|
- `workspace_exists`
|
||||||
|
- `map_workspace_row`
|
||||||
|
- `map_membership_row`
|
||||||
|
|
||||||
|
##### `postgres/invitation.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- invitations lifecycle.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `list_invitations`
|
||||||
|
- `create_invitation`
|
||||||
|
- `delete_invitation`
|
||||||
|
|
||||||
|
##### `postgres/secret.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- secrets;
|
||||||
|
- secret versions;
|
||||||
|
- secret touch/rotation.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `list_secrets`
|
||||||
|
- `get_secret`
|
||||||
|
- `get_current_secret_version`
|
||||||
|
- `create_secret`
|
||||||
|
- `rotate_secret`
|
||||||
|
- `delete_secret`
|
||||||
|
- `touch_secret`
|
||||||
|
|
||||||
|
##### `postgres/stream.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- stream sessions;
|
||||||
|
- async jobs;
|
||||||
|
- state transitions;
|
||||||
|
- cleanup.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `create_stream_session`
|
||||||
|
- `get_stream_session`
|
||||||
|
- `update_stream_session_state`
|
||||||
|
- `close_stream_session`
|
||||||
|
- `list_stream_sessions`
|
||||||
|
- `delete_expired_stream_sessions`
|
||||||
|
- `create_async_job`
|
||||||
|
- `get_async_job`
|
||||||
|
- `update_async_job_status`
|
||||||
|
- `cancel_async_job`
|
||||||
|
- `list_async_jobs`
|
||||||
|
- `delete_expired_async_jobs`
|
||||||
|
|
||||||
|
Internal helpers:
|
||||||
|
|
||||||
|
- `assert_stream_session_transition`
|
||||||
|
- `assert_async_job_transition`
|
||||||
|
- `map_stream_session_row`
|
||||||
|
- `map_async_job_row`
|
||||||
|
|
||||||
|
##### `postgres/api_key.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- platform API key lifecycle.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `list_platform_api_keys`
|
||||||
|
- `get_platform_api_key_by_secret_for_workspace_slug`
|
||||||
|
- `create_platform_api_key`
|
||||||
|
- `revoke_platform_api_key`
|
||||||
|
- `delete_platform_api_key`
|
||||||
|
- `touch_platform_api_key`
|
||||||
|
|
||||||
|
##### `postgres/operation.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- operations;
|
||||||
|
- versions;
|
||||||
|
- published operations;
|
||||||
|
- samples;
|
||||||
|
- descriptors.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `create_operation`
|
||||||
|
- `create_version`
|
||||||
|
- `update_operation_draft`
|
||||||
|
- `archive_operation`
|
||||||
|
- `delete_operation`
|
||||||
|
- `has_published_agent_bindings_for_operation`
|
||||||
|
- `list_operations`
|
||||||
|
- `list_operation_usage_summaries`
|
||||||
|
- `list_operation_agent_refs`
|
||||||
|
- `get_operation_summary`
|
||||||
|
- `get_operation_version`
|
||||||
|
- `list_operation_versions`
|
||||||
|
- `publish_operation`
|
||||||
|
- `get_published_operation`
|
||||||
|
- `list_published_operations`
|
||||||
|
- `save_sample_metadata`
|
||||||
|
- `list_sample_metadata`
|
||||||
|
- `save_descriptor_metadata`
|
||||||
|
- `list_descriptor_metadata`
|
||||||
|
- `insert_version_row`
|
||||||
|
|
||||||
|
Internal helpers:
|
||||||
|
|
||||||
|
- `map_operation_summary_row`
|
||||||
|
- `map_operation_version_row`
|
||||||
|
- `map_published_operation_row`
|
||||||
|
- `serialize_optional_json`
|
||||||
|
|
||||||
|
##### `postgres/agent.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- agents;
|
||||||
|
- agent versions;
|
||||||
|
- bindings;
|
||||||
|
- publish lifecycle.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `list_agents`
|
||||||
|
- `get_agent_summary`
|
||||||
|
- `create_agent`
|
||||||
|
- `get_agent_version`
|
||||||
|
- `save_agent_bindings`
|
||||||
|
- `update_agent_summary`
|
||||||
|
- `delete_agent`
|
||||||
|
- `publish_agent`
|
||||||
|
- `unpublish_agent`
|
||||||
|
- `archive_agent`
|
||||||
|
- `get_published_agent_tools_by_slug`
|
||||||
|
- `list_agent_bindings`
|
||||||
|
- `insert_agent_version_row`
|
||||||
|
- `replace_agent_bindings_rows`
|
||||||
|
|
||||||
|
##### `postgres/observability.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- invocation logs;
|
||||||
|
- usage rollups and breakdowns.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `create_invocation_log`
|
||||||
|
- `list_invocation_logs`
|
||||||
|
- `get_invocation_log`
|
||||||
|
- `summarize_usage`
|
||||||
|
- `list_usage_timeline`
|
||||||
|
- `list_usage_by_operation`
|
||||||
|
- `get_usage_for_operation`
|
||||||
|
- `list_usage_by_agent`
|
||||||
|
- `get_usage_for_agent`
|
||||||
|
|
||||||
|
##### `postgres/yaml_import.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- YAML import jobs.
|
||||||
|
|
||||||
|
Functions:
|
||||||
|
|
||||||
|
- `create_yaml_import_job`
|
||||||
|
- `finish_yaml_import_job`
|
||||||
|
- `get_yaml_import_job`
|
||||||
|
|
||||||
|
##### `postgres/shared.rs`
|
||||||
|
|
||||||
|
Responsibilities:
|
||||||
|
|
||||||
|
- SQL snippets reused by several modules;
|
||||||
|
- common timestamp select fragments during the migration period;
|
||||||
|
- small row-mapping helpers;
|
||||||
|
- pagination helpers.
|
||||||
|
|
||||||
|
Planned content:
|
||||||
|
|
||||||
|
- `Page<T>` helpers if they remain storage-local;
|
||||||
|
- constants for table-independent fragments;
|
||||||
|
- transitional helpers until typed timestamps remove string formatting entirely.
|
||||||
|
|
||||||
|
#### Slice Plan for Track A
|
||||||
|
|
||||||
|
##### Slice A1: File layout and constructors
|
||||||
|
|
||||||
|
- create `postgres/` directory;
|
||||||
|
- move `connect`, `connect_with_options`, `migrate`, and struct wiring;
|
||||||
|
- keep all public signatures unchanged.
|
||||||
|
|
||||||
|
##### Slice A2: Workspace/Auth/API-key split
|
||||||
|
|
||||||
|
- move `auth.rs`, `workspace.rs`, `invitation.rs`, `api_key.rs`;
|
||||||
|
- leave behavior unchanged;
|
||||||
|
- add targeted module tests if not present.
|
||||||
|
|
||||||
|
##### Slice A3: Operation/Agent split
|
||||||
|
|
||||||
|
- move `operation.rs`, `agent.rs`, `yaml_import.rs`;
|
||||||
|
- keep SQL unchanged for this slice;
|
||||||
|
- ensure published-operation and published-agent behavior is unchanged.
|
||||||
|
|
||||||
|
##### Slice A4: Secret/Stream/Observability split
|
||||||
|
|
||||||
|
- move `secret.rs`, `stream.rs`, `observability.rs`;
|
||||||
|
- keep transition logic and cleanup behavior unchanged.
|
||||||
|
|
||||||
|
##### Slice A5: Remove legacy monolith
|
||||||
|
|
||||||
|
- remove remaining methods from root `postgres.rs`;
|
||||||
|
- rename module structure so `postgres/mod.rs` is the only entry point.
|
||||||
|
|
||||||
|
#### Done Criteria for Track A
|
||||||
|
|
||||||
|
- no `postgres.rs` monolith remains;
|
||||||
|
- no single registry source file exceeds roughly 800–1200 lines;
|
||||||
|
- all existing registry tests pass unchanged;
|
||||||
|
- `lib.rs` still re-exports `PostgresRegistry` without downstream breakage.
|
||||||
|
|
||||||
|
## Track B: Compile-Time SQL Verification
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
Most static SQL currently uses `sqlx::query(...)` / `query_as::<_, T>(...)` with runtime validation only. That leaves:
|
||||||
|
|
||||||
|
- column name typos;
|
||||||
|
- mismatched types;
|
||||||
|
- bad nullability assumptions;
|
||||||
|
- schema drift
|
||||||
|
|
||||||
|
to runtime instead of compile time.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Use `sqlx::query!` and `sqlx::query_as!` for every static query with stable shape.
|
||||||
|
|
||||||
|
Keep runtime builders only where the query shape is truly dynamic.
|
||||||
|
|
||||||
|
### Query Classification
|
||||||
|
|
||||||
|
#### Must migrate to `query!` / `query_as!`
|
||||||
|
|
||||||
|
Use macros for:
|
||||||
|
|
||||||
|
- single-row fetches by ID;
|
||||||
|
- list queries with static columns;
|
||||||
|
- insert/update/delete with static statement shape;
|
||||||
|
- existence checks;
|
||||||
|
- count queries;
|
||||||
|
- cleanup queries with fixed predicates.
|
||||||
|
|
||||||
|
Expected modules:
|
||||||
|
|
||||||
|
- `auth.rs`
|
||||||
|
- `workspace.rs`
|
||||||
|
- `invitation.rs`
|
||||||
|
- `secret.rs`
|
||||||
|
- `stream.rs`
|
||||||
|
- `api_key.rs`
|
||||||
|
- most of `agent.rs`
|
||||||
|
- most of `yaml_import.rs`
|
||||||
|
|
||||||
|
#### May stay runtime-based
|
||||||
|
|
||||||
|
Keep dynamic APIs only when necessary:
|
||||||
|
|
||||||
|
- optional filters that materially change SQL shape;
|
||||||
|
- optional sort columns chosen from a whitelist;
|
||||||
|
- bulk dynamic `IN` lists where macro usage becomes unreadable;
|
||||||
|
- `QueryBuilder`-based multi-row inserts/updates.
|
||||||
|
|
||||||
|
Expected modules:
|
||||||
|
|
||||||
|
- `operation.rs` list queries with optional filters;
|
||||||
|
- `observability.rs` usage aggregations and breakdowns;
|
||||||
|
- some search/filter endpoints.
|
||||||
|
|
||||||
|
### Required Infrastructure
|
||||||
|
|
||||||
|
#### Add SQLx offline support
|
||||||
|
|
||||||
|
Repository-level requirements:
|
||||||
|
|
||||||
|
- use `sqlx prepare` to generate checked metadata;
|
||||||
|
- keep checked metadata in version control if the team wants deterministic CI;
|
||||||
|
- wire verification into CI or `just verify`.
|
||||||
|
|
||||||
|
Concrete additions:
|
||||||
|
|
||||||
|
- `sqlx-data.json` or the current SQLx metadata equivalent for the chosen version;
|
||||||
|
- `DATABASE_URL` or test DB wiring for `cargo sqlx prepare`.
|
||||||
|
|
||||||
|
#### Introduce row structs where needed
|
||||||
|
|
||||||
|
For `query_as!`, add storage-local row structs for cases where domain structs should not be coupled directly to SQL column shape.
|
||||||
|
|
||||||
|
Examples:
|
||||||
|
|
||||||
|
- `WorkspaceRow`
|
||||||
|
- `AuthUserRow`
|
||||||
|
- `OperationSummaryRow`
|
||||||
|
- `InvocationLogRow`
|
||||||
|
- `UsageTimelineRow`
|
||||||
|
|
||||||
|
Rule:
|
||||||
|
|
||||||
|
- domain structs remain domain-oriented;
|
||||||
|
- row structs remain SQL-oriented.
|
||||||
|
|
||||||
|
### Function-Level Migration Rules
|
||||||
|
|
||||||
|
#### For `query_as!`
|
||||||
|
|
||||||
|
Use `query_as!` when:
|
||||||
|
|
||||||
|
- the query result maps cleanly to a typed row struct;
|
||||||
|
- all returned columns are explicit;
|
||||||
|
- nullability is known.
|
||||||
|
|
||||||
|
#### For `query!`
|
||||||
|
|
||||||
|
Use `query!` when:
|
||||||
|
|
||||||
|
- no reusable row struct is needed;
|
||||||
|
- the query is a simple `insert`, `update`, `delete`, or `exists` fetch.
|
||||||
|
|
||||||
|
#### For dynamic list queries
|
||||||
|
|
||||||
|
Use:
|
||||||
|
|
||||||
|
- `sqlx::QueryBuilder<Postgres>` with explicit allowed sort/filter sets;
|
||||||
|
- storage-local mapper functions from row to domain object.
|
||||||
|
|
||||||
|
### Migration Order
|
||||||
|
|
||||||
|
##### Slice B1: `auth`, `workspace`, `invitation`
|
||||||
|
|
||||||
|
Low-risk, fixed-shape queries.
|
||||||
|
|
||||||
|
##### Slice B2: `secret`, `stream`, `api_key`, `yaml_import`
|
||||||
|
|
||||||
|
Still mostly fixed-shape.
|
||||||
|
|
||||||
|
##### Slice B3: `operation`, `agent`
|
||||||
|
|
||||||
|
Introduce mixed strategy: macros for fixed queries, builder for dynamic lists.
|
||||||
|
|
||||||
|
##### Slice B4: `observability`
|
||||||
|
|
||||||
|
Likely the most dynamic area. Convert only static pieces first, leave complex aggregations to `QueryBuilder`.
|
||||||
|
|
||||||
|
### Done Criteria for Track B
|
||||||
|
|
||||||
|
- all fixed-shape SQL uses compile-time checked macros;
|
||||||
|
- dynamic SQL is explicitly documented and isolated;
|
||||||
|
- CI fails on SQL drift before runtime tests;
|
||||||
|
- no new static query is accepted through `sqlx::query(...)` without justification.
|
||||||
|
|
||||||
|
## Track C: SecretCrypto KDF Correction
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
Current code in `crates/crank-runtime/src/secret_crypto.rs` derives the AES key by hashing the master key with SHA-256:
|
||||||
|
|
||||||
|
- no KDF semantics;
|
||||||
|
- no labeled derivation context;
|
||||||
|
- no future rotation/versioning path.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Replace direct hashing with HKDF-SHA256.
|
||||||
|
|
||||||
|
### Target Design
|
||||||
|
|
||||||
|
`SecretCrypto::new(master_key: &str)` will:
|
||||||
|
|
||||||
|
1. validate non-empty master key;
|
||||||
|
2. derive a 32-byte AES key using `Hkdf<Sha256>`;
|
||||||
|
3. use explicit `info` context, for example:
|
||||||
|
- `b"crank.secret-envelope.v1"`
|
||||||
|
4. initialize `Aes256Gcm` from the derived bytes.
|
||||||
|
|
||||||
|
### Module Changes
|
||||||
|
|
||||||
|
File:
|
||||||
|
|
||||||
|
- `crates/crank-runtime/src/secret_crypto.rs`
|
||||||
|
|
||||||
|
Changes:
|
||||||
|
|
||||||
|
- replace `Sha256::digest(trimmed.as_bytes())` with HKDF expansion;
|
||||||
|
- preserve the public constructor signature;
|
||||||
|
- keep ciphertext format unchanged if possible;
|
||||||
|
- if ciphertext format must change later, introduce envelope `version` before doing so.
|
||||||
|
|
||||||
|
### Required Tests
|
||||||
|
|
||||||
|
- `encrypt_decrypt_roundtrip_still_works`
|
||||||
|
- `empty_master_key_is_rejected`
|
||||||
|
- `same_master_key_derives_stable_key`
|
||||||
|
- `different_master_keys_produce_different_ciphertexts`
|
||||||
|
|
||||||
|
### Done Criteria for Track C
|
||||||
|
|
||||||
|
- no direct SHA-256 KDF remains;
|
||||||
|
- `SecretCrypto` uses HKDF-SHA256 with explicit context;
|
||||||
|
- all existing secret encrypt/decrypt flows remain compatible.
|
||||||
|
|
||||||
|
## Track D: Typed Timestamps
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
Domain and storage structs use `String` timestamps. SQL converts timestamps via repeated `to_char(...)`.
|
||||||
|
|
||||||
|
That causes:
|
||||||
|
|
||||||
|
- repeated formatting logic in SQL;
|
||||||
|
- weak type guarantees;
|
||||||
|
- inability to do time arithmetic without reparsing;
|
||||||
|
- broad surface for formatting drift.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Use `time::OffsetDateTime` in domain and storage models, with RFC 3339 serialization at the transport boundary.
|
||||||
|
|
||||||
|
### Target Type Strategy
|
||||||
|
|
||||||
|
#### Domain layer
|
||||||
|
|
||||||
|
Use:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
#[serde(with = "time::serde::rfc3339")]
|
||||||
|
pub created_at: OffsetDateTime
|
||||||
|
```
|
||||||
|
|
||||||
|
and the same for `updated_at`, `expires_at`, `used_at`, and similar fields.
|
||||||
|
|
||||||
|
#### Storage row layer
|
||||||
|
|
||||||
|
Also use `OffsetDateTime`.
|
||||||
|
|
||||||
|
Do not format timestamps in SQL except for transitional compatibility shims.
|
||||||
|
|
||||||
|
#### Transport layer
|
||||||
|
|
||||||
|
JSON still emits RFC 3339 strings through Serde.
|
||||||
|
|
||||||
|
### Files Affected
|
||||||
|
|
||||||
|
#### Domain crates
|
||||||
|
|
||||||
|
- `crates/crank-core/src/workspace.rs`
|
||||||
|
- `crates/crank-core/src/access.rs`
|
||||||
|
- `crates/crank-core/src/auth.rs`
|
||||||
|
- `crates/crank-core/src/agent.rs`
|
||||||
|
- `crates/crank-core/src/operation.rs`
|
||||||
|
- `crates/crank-core/src/observability.rs`
|
||||||
|
- `crates/crank-core/src/secret.rs`
|
||||||
|
- `crates/crank-core/src/stream_session.rs`
|
||||||
|
|
||||||
|
#### Registry models
|
||||||
|
|
||||||
|
- `crates/crank-registry/src/model.rs`
|
||||||
|
|
||||||
|
#### Admin API transport/service DTOs
|
||||||
|
|
||||||
|
- `apps/admin-api/src/service.rs`
|
||||||
|
- any route DTO files that mirror time fields directly
|
||||||
|
|
||||||
|
### Migration Strategy
|
||||||
|
|
||||||
|
##### Slice D1: storage row types
|
||||||
|
|
||||||
|
- change storage row structs to `OffsetDateTime`;
|
||||||
|
- remove `to_char(...)` from fixed-shape SQL;
|
||||||
|
- keep service DTOs as strings temporarily if needed.
|
||||||
|
|
||||||
|
##### Slice D2: domain models
|
||||||
|
|
||||||
|
- convert `crank-core` models to typed timestamps with Serde helpers;
|
||||||
|
- update constructors and tests.
|
||||||
|
|
||||||
|
##### Slice D3: API/service transport
|
||||||
|
|
||||||
|
- remove string timestamp shims from service DTOs where possible;
|
||||||
|
- ensure JSON remains stable.
|
||||||
|
|
||||||
|
##### Slice D4: cleanup
|
||||||
|
|
||||||
|
- remove transitional formatting helpers;
|
||||||
|
- remove `now_string()` helpers in favor of typed timestamps.
|
||||||
|
|
||||||
|
### Acceptance Criteria
|
||||||
|
|
||||||
|
- no `created_at: String` or `updated_at: String` remains in domain/storage models;
|
||||||
|
- no `to_char(created_at...)` / `to_char(updated_at...)` remains in registry SQL except perhaps in legacy compatibility code slated for removal in the same slice;
|
||||||
|
- JSON contract stays RFC 3339.
|
||||||
|
|
||||||
|
## Track E: `Display` for ID Newtypes
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
`define_id!` currently requires `.as_str()` everywhere for formatting and messages.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Add `std::fmt::Display` support to every generated ID newtype.
|
||||||
|
|
||||||
|
### File
|
||||||
|
|
||||||
|
- `crates/crank-core/src/ids.rs`
|
||||||
|
|
||||||
|
### Change
|
||||||
|
|
||||||
|
Add inside `define_id!`:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
impl std::fmt::Display for $name {
|
||||||
|
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||||
|
write!(f, "{}", self.0)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### Follow-Up Cleanup
|
||||||
|
|
||||||
|
After macro support lands:
|
||||||
|
|
||||||
|
- replace formatting sites like `workspace_id.as_str()` inside messages;
|
||||||
|
- keep `.as_str()` only where an actual `&str` API requires it.
|
||||||
|
|
||||||
|
### Acceptance Criteria
|
||||||
|
|
||||||
|
- all ID newtypes implement `Display`;
|
||||||
|
- formatting code no longer uses `.as_str()` inside `format!`, `println!`, or error messages unless `&str` is actually required.
|
||||||
|
|
||||||
|
## Track F: PostgreSQL Pool Configuration
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
The pool is currently created via default `PgPoolOptions`, which means the runtime behavior is implicit and under-configured.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Make pool behavior explicit and configurable from environment.
|
||||||
|
|
||||||
|
### Target Config Model
|
||||||
|
|
||||||
|
Add a typed config struct, for example:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
pub struct PostgresPoolConfig {
|
||||||
|
pub max_connections: u32,
|
||||||
|
pub min_connections: u32,
|
||||||
|
pub acquire_timeout_ms: u64,
|
||||||
|
pub idle_timeout_ms: u64,
|
||||||
|
pub max_lifetime_ms: u64,
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### Runtime Environment
|
||||||
|
|
||||||
|
Add env variables:
|
||||||
|
|
||||||
|
- `POSTGRES_MAX_CONNECTIONS`
|
||||||
|
- `POSTGRES_MIN_CONNECTIONS`
|
||||||
|
- `POSTGRES_ACQUIRE_TIMEOUT_MS`
|
||||||
|
- `POSTGRES_IDLE_TIMEOUT_MS`
|
||||||
|
- `POSTGRES_MAX_LIFETIME_MS`
|
||||||
|
|
||||||
|
### Files
|
||||||
|
|
||||||
|
- `crates/crank-registry/src/postgres/connect.rs`
|
||||||
|
- `apps/admin-api/src/main.rs`
|
||||||
|
- `apps/mcp-server/src/main.rs`
|
||||||
|
- config docs and examples
|
||||||
|
|
||||||
|
### Behavior
|
||||||
|
|
||||||
|
`connect_with_options` should accept either:
|
||||||
|
|
||||||
|
- direct `PgConnectOptions` plus explicit pool config;
|
||||||
|
- or an internal default config built from env.
|
||||||
|
|
||||||
|
Defaults should be conservative and explicit, for example:
|
||||||
|
|
||||||
|
- `max_connections = 20`
|
||||||
|
- `min_connections = 2`
|
||||||
|
- `acquire_timeout = 5s`
|
||||||
|
- `idle_timeout = 10m`
|
||||||
|
- `max_lifetime = 30m`
|
||||||
|
|
||||||
|
Exact values can be tuned later, but they must no longer be hidden defaults.
|
||||||
|
|
||||||
|
### Acceptance Criteria
|
||||||
|
|
||||||
|
- pool limits and timeouts are explicit;
|
||||||
|
- docs reflect the new variables;
|
||||||
|
- startup logs or config inspection make active pool settings visible.
|
||||||
|
|
||||||
|
## Track G: Timestamp Formatting Deduplication
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
The same `to_char(... at time zone 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"')` fragment is copied across many queries.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Eliminate duplicated formatting logic.
|
||||||
|
|
||||||
|
### Strategy
|
||||||
|
|
||||||
|
This track is mostly resolved by Track D, but there is a transition window.
|
||||||
|
|
||||||
|
During transition:
|
||||||
|
|
||||||
|
- centralize any remaining formatting in `postgres/shared.rs`;
|
||||||
|
- define named SQL fragments only once;
|
||||||
|
- remove inline duplicated snippets.
|
||||||
|
|
||||||
|
After Track D:
|
||||||
|
|
||||||
|
- remove formatting fragments entirely.
|
||||||
|
|
||||||
|
### Acceptance Criteria
|
||||||
|
|
||||||
|
- duplicated inline timestamp formatting is no longer scattered across query strings;
|
||||||
|
- final state has typed timestamps instead of SQL formatting.
|
||||||
|
|
||||||
|
## Track H: Error Structure Normalization
|
||||||
|
|
||||||
|
### Problem
|
||||||
|
|
||||||
|
Some runtime and API error variants carry only a `details: String`, which makes programmatic handling and consistent observability harder.
|
||||||
|
|
||||||
|
### Goal
|
||||||
|
|
||||||
|
Introduce structured error payloads where the current model is too string-heavy.
|
||||||
|
|
||||||
|
### Scope
|
||||||
|
|
||||||
|
Priority types:
|
||||||
|
|
||||||
|
- `RuntimeError`
|
||||||
|
- registry storage errors where context is stringly typed;
|
||||||
|
- API error mapping around secret/runtime/streaming faults.
|
||||||
|
|
||||||
|
### Design Rules
|
||||||
|
|
||||||
|
- keep `thiserror`;
|
||||||
|
- use structured fields in variants whenever stable context exists;
|
||||||
|
- reserve free-form `details` only for opaque third-party messages.
|
||||||
|
|
||||||
|
Examples:
|
||||||
|
|
||||||
|
Instead of:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
RuntimeError::SecretCrypto { details: String }
|
||||||
|
```
|
||||||
|
|
||||||
|
prefer:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
RuntimeError::SecretCrypto {
|
||||||
|
operation: &'static str,
|
||||||
|
details: String,
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
or dedicated variants such as:
|
||||||
|
|
||||||
|
- `InvalidSecretEnvelope`
|
||||||
|
- `SecretDecryptFailed`
|
||||||
|
- `SecretSerializeFailed`
|
||||||
|
|
||||||
|
### Slice Plan
|
||||||
|
|
||||||
|
##### Slice H1
|
||||||
|
|
||||||
|
- normalize `SecretCrypto` and streaming/session/job errors.
|
||||||
|
|
||||||
|
##### Slice H2
|
||||||
|
|
||||||
|
- normalize adapter errors for REST/gRPC/WebSocket/SOAP where stable categories exist.
|
||||||
|
|
||||||
|
##### Slice H3
|
||||||
|
|
||||||
|
- normalize API mapping and error codes.
|
||||||
|
|
||||||
|
### Acceptance Criteria
|
||||||
|
|
||||||
|
- core runtime error variants expose structured context;
|
||||||
|
- API can map key failure classes deterministically;
|
||||||
|
- logs and telemetry can group errors by category instead of by ad hoc message text.
|
||||||
|
|
||||||
|
## Delivery Order
|
||||||
|
|
||||||
|
Recommended order:
|
||||||
|
|
||||||
|
1. `feat/refactoring-plan`
|
||||||
|
2. `feat/id-display-support`
|
||||||
|
3. `feat/postgres-pool-config`
|
||||||
|
4. `feat/postgres-registry-modularization`
|
||||||
|
5. `feat/sqlx-compile-time-verification`
|
||||||
|
6. `feat/secret-crypto-hkdf`
|
||||||
|
7. `feat/typed-timestamps`
|
||||||
|
8. `feat/error-structure-normalization`
|
||||||
|
9. `feat/post-refactor-cleanup`
|
||||||
|
|
||||||
|
Reasoning:
|
||||||
|
|
||||||
|
- `Display` is tiny and immediately reduces noise.
|
||||||
|
- pool config is operational and isolated.
|
||||||
|
- module split should happen before large query migrations, otherwise the macro migration lands into the monolith and becomes harder to review.
|
||||||
|
- HKDF is isolated and low-risk.
|
||||||
|
- typed timestamps should land after storage modules are split and query migration is underway.
|
||||||
|
|
||||||
|
## Per-Slice Verification
|
||||||
|
|
||||||
|
Every slice must run:
|
||||||
|
|
||||||
|
- `just fmt-check`
|
||||||
|
- `just check`
|
||||||
|
- `just clippy`
|
||||||
|
- targeted tests for the touched area
|
||||||
|
|
||||||
|
Additionally:
|
||||||
|
|
||||||
|
- SQL macro migration slices must run SQLx prepare/verification;
|
||||||
|
- timestamp slices must run serialization regression tests;
|
||||||
|
- secret crypto slice must run compatibility and roundtrip tests.
|
||||||
|
|
||||||
|
## Success Definition
|
||||||
|
|
||||||
|
The refactoring track is complete when:
|
||||||
|
|
||||||
|
- registry storage code is split into domain modules;
|
||||||
|
- static SQL is compile-time validated;
|
||||||
|
- `SecretCrypto` uses HKDF instead of SHA-256 as a KDF;
|
||||||
|
- domain/storage timestamps are typed;
|
||||||
|
- ID newtypes implement `Display`;
|
||||||
|
- PostgreSQL pool settings are explicit and configurable;
|
||||||
|
- timestamp formatting duplication is removed;
|
||||||
|
- error variants carry structured context where it matters.
|
||||||
|
|
||||||
|
At that point, maintainability improves materially without changing the product model or feature scope.
|
||||||
Reference in New Issue
Block a user