From 121573d91f2e9de4466dcf8b6d1a1881326e4e52 Mon Sep 17 00:00:00 2001 From: Rita Agafonova <36133358+rita-aga@users.noreply.github.com> Date: Tue, 14 Jul 2026 11:14:19 -0700 Subject: [PATCH 1/3] test(server): failing tests for feature-request read side effects (ARN-240) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RED: GET /observe/evolution/feature-requests regenerates feature requests on every read, and each generated record mints a fresh UUID-suffixed id — so the store "upsert" inserts a NEW row per GET, a fresh FR-{uuid} system entity is dispatched per generated record per GET, and the upsert overwrites developer-owned disposition and notes with generator defaults. Three tests pin all three legs against the real turso-backed stack: row cardinality + identity stability across reads, developer-field preservation, and exactly one creation event on the record's entity journal. Co-Authored-By: Claude Fable 5 --- .../src/observe/evolution/operations.rs | 3 + .../operations/feature_requests_test.rs | 236 ++++++++++++++++++ 2 files changed, 239 insertions(+) create mode 100644 crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs diff --git a/crates/temper-server/src/observe/evolution/operations.rs b/crates/temper-server/src/observe/evolution/operations.rs index b6e06a4ed..358f3cb04 100644 --- a/crates/temper-server/src/observe/evolution/operations.rs +++ b/crates/temper-server/src/observe/evolution/operations.rs @@ -21,6 +21,9 @@ use crate::state::{ObserveRefreshHint, ServerState}; mod materialize; mod support; +#[cfg(test)] +mod feature_requests_test; + pub(crate) use materialize::{handle_evolution_analyze, handle_evolution_materialize}; use support::{ diff --git a/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs b/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs new file mode 100644 index 000000000..9b7a1dd98 --- /dev/null +++ b/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs @@ -0,0 +1,236 @@ +//! ARN-240: GET /observe/evolution/feature-requests must be idempotent. +//! +//! The handler generates feature requests from trajectory gaps on every read. +//! Each generated record minted a fresh UUID-suffixed id, so the store +//! "upsert" inserted a NEW row per GET, and a fresh `FR-{uuid}` system entity +//! was dispatched per generated record per GET — reads spawned unbounded +//! duplicates, and re-generation clobbered developer-owned fields. + +use std::collections::BTreeMap; + +use crate::registry::SpecRegistry; +use axum::extract::{Query, State}; +use axum::http::HeaderMap; +use temper_runtime::ActorSystem; + +use crate::state::{ServerState, TrajectoryEntry, TrajectorySource}; +use crate::storage::StorageStack; + +fn failing_platform_entry(n: u64) -> TrajectoryEntry { + TrajectoryEntry { + timestamp: format!("2026-07-13T00:00:{n:02}Z"), + tenant: "arn240".to_string(), + entity_type: "Invoice".to_string(), + entity_id: format!("inv-{n}"), + action: "GenerateInvoice".to_string(), + success: false, + from_status: None, + to_status: None, + error: Some("EntitySetNotFound: Invoice".to_string()), + agent_id: Some("agent-1".to_string()), + session_id: None, + authz_denied: None, + denied_resource: None, + denied_module: None, + source: Some(TrajectorySource::Platform), + spec_governed: Some(false), + agent_type: None, + intent: None, + request_body: None, + matched_policy_ids: None, + } +} + +const FEATURE_REQUEST_IOA: &str = r#" +[automaton] +name = "FeatureRequest" +states = ["New", "Ready"] +initial = "New" + +[[state]] +name = "category" +type = "string" +initial = "" + +[[state]] +name = "description" +type = "string" +initial = "" + +[[state]] +name = "frequency" +type = "string" +initial = "" + +[[state]] +name = "developer_notes" +type = "string" +initial = "" + +[[state]] +name = "legacy_record_id" +type = "string" +initial = "" + +[[action]] +name = "CreateFeatureRequest" +kind = "input" +from = ["New"] +to = "Ready" +params = ["category", "description", "frequency", "developer_notes", "legacy_record_id"] +hint = "Record a platform gap surfaced by the insight generator." +"#; + +const FEATURE_REQUEST_CSDL: &str = r#" + + + + + + + + + + + + +"#; + +fn registry_with_system_feature_request_spec() -> SpecRegistry { + let mut registry = SpecRegistry::new(); + let csdl = temper_spec::parse_csdl(FEATURE_REQUEST_CSDL).expect("csdl parses"); + registry.register_tenant( + "temper-system", + csdl, + FEATURE_REQUEST_CSDL.to_string(), + &[("FeatureRequest", FEATURE_REQUEST_IOA)], + ); + registry +} + +async fn state_with_gap_trajectories() -> (ServerState, tempfile::TempDir) { + let dir = tempfile::tempdir().expect("tempdir"); + let db_url = format!("file:{}", dir.path().join("arn240.db").display()); + let turso = temper_store_turso::TursoEventStore::new(&db_url, None) + .await + .expect("turso store"); + let stack = StorageStack::from_turso(turso); + + // Three failing Platform-source entries for the same (action, error + // pattern) — exactly the FEATURE_REQUEST_THRESHOLD gap group. + let sink = stack.trajectory.clone().expect("trajectory sink"); + for n in 0..3 { + sink.persist_trajectory_entry(&failing_platform_entry(n)) + .await + .expect("persist trajectory"); + } + + let system = ActorSystem::new("arn240-test"); + let mut state = ServerState::from_registry(system, registry_with_system_feature_request_spec()); + state.set_storage_stack(stack); + (state, dir) +} + +fn system_headers() -> HeaderMap { + let mut headers = HeaderMap::new(); + headers.insert("x-temper-principal-kind", "system".parse().expect("hdr")); + headers +} + +async fn get_feature_requests(state: &ServerState) -> serde_json::Value { + let response = super::handle_feature_requests( + State(state.clone()), + system_headers(), + Query(BTreeMap::new()), + ) + .await + .expect("GET feature-requests"); + response.0 +} + +/// A read must not create anything new on re-read: the same gap group must +/// map to the same feature request, however many times it is listed. +#[tokio::test] +async fn repeated_get_does_not_duplicate_feature_requests() { + let (state, _dir) = state_with_gap_trajectories().await; + + let first = get_feature_requests(&state).await; + assert_eq!( + first["total"], 1, + "one gap group must yield one feature request, got: {first}" + ); + + let second = get_feature_requests(&state).await; + assert_eq!( + second["total"], 1, + "a GET is a read — re-reading must not create a duplicate feature \ + request for the same gap group, got: {second}" + ); + assert_eq!( + second["feature_requests"][0]["id"], first["feature_requests"][0]["id"], + "the same gap group must keep the same identity across reads" + ); +} + +/// Re-generation must not clobber developer-owned fields: a disposition set +/// via PATCH survives subsequent GETs while agents keep hitting the same gap. +#[tokio::test] +async fn get_preserves_developer_disposition_and_notes() { + let (state, _dir) = state_with_gap_trajectories().await; + + let first = get_feature_requests(&state).await; + let id = first["feature_requests"][0]["id"] + .as_str() + .expect("feature request id") + .to_string(); + + let store = state.platform_metadata_store().expect("platform store"); + store + .update_feature_request(&id, "WontFix", Some("duplicate of FR-1")) + .await + .expect("developer updates disposition"); + + let after = get_feature_requests(&state).await; + assert_eq!(after["total"], 1, "still exactly one row, got: {after}"); + assert_eq!( + after["feature_requests"][0]["disposition"], "WontFix", + "a GET must not reset a developer's disposition, got: {after}" + ); + assert_eq!( + after["feature_requests"][0]["developer_notes"], "duplicate of FR-1", + "a GET must not wipe developer notes, got: {after}" + ); +} + +/// The system entity behind a feature request is created ONCE — the entity +/// journal for the record's deterministic id holds exactly one creation +/// event however many times the listing runs. (Previously every GET +/// dispatched a fresh `FR-{uuid}` entity per generated record.) +#[tokio::test] +async fn repeated_get_creates_the_system_entity_exactly_once() { + let (state, _dir) = state_with_gap_trajectories().await; + + let first = get_feature_requests(&state).await; + let id = first["feature_requests"][0]["id"] + .as_str() + .expect("feature request id") + .to_string(); + get_feature_requests(&state).await; + + let events = state + .storage_stack + .as_ref() + .expect("stack") + .events + .read_events(&format!("temper-system:FeatureRequest:{id}"), 0) + .await + .expect("read entity journal"); + assert_eq!( + events.len(), + 1, + "two GETs must leave exactly one creation event on the record's \ + entity journal, got {} (ids minted per read would journal under \ + fresh ids and leave this journal empty or duplicated)", + events.len() + ); +} From 2d70eccd2d08285b5f5bfc2a70b913944fa7132e Mon Sep 17 00:00:00 2001 From: Rita Agafonova <36133358+rita-aga@users.noreply.github.com> Date: Tue, 14 Jul 2026 13:55:11 -0700 Subject: [PATCH 2/3] fix(server): make feature-request reads idempotent via content-derived identity (ARN-240) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GREEN: a feature request's id is now a stable hash of its gap group key — FR-{sha256(action, error_pattern)[..12]} — instead of a UUID minted per generation, so the regeneration that runs inside every GET converges on the same record instead of inserting a duplicate per read. The store upsert is split into INSERT ... ON CONFLICT DO NOTHING plus a generator-fields-only UPDATE in both backends (returning whether it inserted), which also stops regeneration from clobbering the developer-owned disposition and notes. The system entity is dispatched once — on the read that first discovered the gap — and shares the record's id; the per-read FR-{uuid} dispatch is gone. The RED entity-journal test expected exactly one event; the GREEN run revealed a fresh entity journals a bootstrap Created event plus the action event, so the assertion now pins the real invariant instead: journal non-empty under the record's id after the first read, identity stable, and journal length unchanged by later reads (still failing at the RED base, where dispatches went to unrelated random ids). The +4 trait-doc lines pushed storage/mod.rs over the readability ceiling; the three capability traits (EvolutionStore, DesignTimeEventStore, OtsStore) moved verbatim to storage/capabilities.rs (2833 -> 2715 lines). ADR-0163 records the decision, the benign concurrent-GET race, the at-most-once entity dispatch residual, and why generation stays in the GET for now. Co-Authored-By: Claude Fable 5 --- .../insight_generator/gap_analysis.rs | 22 ++- .../src/observe/evolution/operations.rs | 47 ++++--- .../operations/feature_requests_test.rs | 50 ++++--- .../temper-server/src/storage/capabilities.rs | 129 ++++++++++++++++++ crates/temper-server/src/storage/mod.rs | 122 +---------------- crates/temper-store-postgres/src/platform.rs | 33 +++-- .../temper-store-turso/src/store/evolution.rs | 28 +++- .../0163-feature-request-read-idempotency.md | 70 ++++++++++ 8 files changed, 332 insertions(+), 169 deletions(-) create mode 100644 crates/temper-server/src/storage/capabilities.rs create mode 100644 docs/adrs/0163-feature-request-read-idempotency.md diff --git a/crates/temper-server/src/observe/evolution/insight_generator/gap_analysis.rs b/crates/temper-server/src/observe/evolution/insight_generator/gap_analysis.rs index bcd6a998b..8d83c8146 100644 --- a/crates/temper-server/src/observe/evolution/insight_generator/gap_analysis.rs +++ b/crates/temper-server/src/observe/evolution/insight_generator/gap_analysis.rs @@ -153,6 +153,20 @@ pub(crate) fn generate_unmet_intents( const FEATURE_REQUEST_THRESHOLD: u64 = 3; +/// Stable, content-derived feature-request id: the same platform gap always +/// maps to the same record (ARN-240). 12 hex chars of SHA-256 over the gap +/// group key, NUL-separated to prevent field-boundary ambiguity. +fn deterministic_feature_request_id(action: &str, error_pattern: &str) -> String { + use sha2::{Digest, Sha256}; + let mut hasher = Sha256::new(); + hasher.update(action.as_bytes()); + hasher.update([0]); + hasher.update(error_pattern.as_bytes()); + let digest = hasher.finalize(); + let hex: String = digest.iter().map(|b| format!("{b:02x}")).collect(); + format!("FR-{}", &hex[..12]) +} + pub(crate) fn generate_feature_requests( entries: &[crate::state::TrajectoryEntry], ) -> Vec { @@ -196,8 +210,14 @@ pub(crate) fn generate_feature_requests( _ => PlatformGapCategory::MissingCapability, }; + let mut header = RecordHeader::new(RecordType::FeatureRequest, "insight-generator"); + // ARN-240: identity derives from the gap group, not a minted UUID — + // the same (action, error pattern) always maps to the same record, so + // regeneration on every listing is idempotent by construction. + header.id = deterministic_feature_request_id(&accum.action, &accum.error_pattern); + feature_requests.push(FeatureRequestRecord { - header: RecordHeader::new(RecordType::FeatureRequest, "insight-generator"), + header, category, description: format!( "Agents tried '{}' {} times — {}", diff --git a/crates/temper-server/src/observe/evolution/operations.rs b/crates/temper-server/src/observe/evolution/operations.rs index 358f3cb04..cb308305b 100644 --- a/crates/temper-server/src/observe/evolution/operations.rs +++ b/crates/temper-server/src/observe/evolution/operations.rs @@ -27,8 +27,8 @@ mod feature_requests_test; pub(crate) use materialize::{handle_evolution_analyze, handle_evolution_materialize}; use support::{ - create_system_entity_logged, emit_refresh_hints, next_system_entity_id, persist_alerts, - persist_insights, spawn_intent_discovery, + create_system_entity_logged, emit_refresh_hints, persist_alerts, persist_insights, + spawn_intent_discovery, }; /// POST /api/evolution/sentinel/check -- trigger sentinel rule evaluation. @@ -245,7 +245,10 @@ pub(crate) async fn handle_feature_requests( FeatureRequestDisposition::WontFix => "WontFix", FeatureRequestDisposition::Resolved => "Resolved", }; - if let Err(error) = store + // ARN-240: the record id is content-derived, so this is a true + // upsert; the system entity is created ONCE, on the insert that + // first discovered the gap, and shares the record's id. + let inserted = match store .upsert_feature_request( &feature_request.header.id, &format!("{:?}", feature_request.category), @@ -257,23 +260,29 @@ pub(crate) async fn handle_feature_requests( ) .await { - tracing::warn!(error = %error, backend = store.backend_name(), "failed to upsert feature request"); - } + Ok(inserted) => inserted, + Err(error) => { + tracing::warn!(error = %error, backend = store.backend_name(), "failed to upsert feature request"); + false + } + }; - create_system_entity_logged( - &state, - "FeatureRequest", - &next_system_entity_id("FR"), - "CreateFeatureRequest", - serde_json::json!({ - "category": format!("{:?}", feature_request.category), - "description": feature_request.description, - "frequency": feature_request.frequency.to_string(), - "developer_notes": feature_request.developer_notes.clone().unwrap_or_default(), - "legacy_record_id": feature_request.header.id, - }), - ) - .await; + if inserted { + create_system_entity_logged( + &state, + "FeatureRequest", + &feature_request.header.id, + "CreateFeatureRequest", + serde_json::json!({ + "category": format!("{:?}", feature_request.category), + "description": feature_request.description, + "frequency": feature_request.frequency.to_string(), + "developer_notes": feature_request.developer_notes.clone().unwrap_or_default(), + "legacy_record_id": feature_request.header.id, + }), + ) + .await; + } } return match store.list_feature_requests(disposition_filter).await { diff --git a/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs b/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs index 9b7a1dd98..28c6feb89 100644 --- a/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs +++ b/crates/temper-server/src/observe/evolution/operations/feature_requests_test.rs @@ -215,22 +215,40 @@ async fn repeated_get_creates_the_system_entity_exactly_once() { .as_str() .expect("feature request id") .to_string(); - get_feature_requests(&state).await; - - let events = state - .storage_stack - .as_ref() - .expect("stack") - .events - .read_events(&format!("temper-system:FeatureRequest:{id}"), 0) - .await - .expect("read entity journal"); + + let journal = |from: u64| { + let events = state.storage_stack.as_ref().expect("stack").events.clone(); + let persistence_id = format!("temper-system:FeatureRequest:{id}"); + async move { + events + .read_events(&persistence_id, from) + .await + .expect("read entity journal") + } + }; + + // The first GET must journal the entity UNDER THE RECORD'S ID (a fresh + // per-read id would leave this journal empty). A new entity journals a + // bootstrap Created event plus the action event, so assert non-empty + // rather than a count coupled to that implementation detail. + let after_first = journal(0).await; + assert!( + !after_first.is_empty(), + "the first GET must create the system entity under the record's id" + ); + + let second = get_feature_requests(&state).await; + assert_eq!( + second["feature_requests"][0]["id"].as_str(), + Some(id.as_str()), + "identity must be stable before comparing journals" + ); + + let after_second = journal(0).await; assert_eq!( - events.len(), - 1, - "two GETs must leave exactly one creation event on the record's \ - entity journal, got {} (ids minted per read would journal under \ - fresh ids and leave this journal empty or duplicated)", - events.len() + after_second.len(), + after_first.len(), + "the second GET must not journal anything — the entity is created \ + exactly once, on the read that first discovered the gap" ); } diff --git a/crates/temper-server/src/storage/capabilities.rs b/crates/temper-server/src/storage/capabilities.rs new file mode 100644 index 000000000..e9e8fd19f --- /dev/null +++ b/crates/temper-server/src/storage/capabilities.rs @@ -0,0 +1,129 @@ +//! Durable metadata capability traits for the evolution engine and +//! design-time verification events, implemented by the storage backends. + +use temper_runtime::persistence::PersistenceError; +use temper_store_turso::{ + DesignTimeEventRow, EvolutionRecordRow, FeatureRequestRow, OtsQueuedTrajectoryRow, + OtsTrajectoryParams, OtsTrajectoryRow, +}; + +/// Evolution engine durable metadata capability. +#[async_trait::async_trait] +pub trait EvolutionStore: Send + Sync { + /// Insert a generated feature request, or refresh the generator-owned + /// fields of an existing one. Developer-owned fields (disposition, + /// developer notes) are written ONLY on first insert (ARN-240). Returns + /// `true` when this call created the record. + #[allow(clippy::too_many_arguments)] + async fn upsert_feature_request( + &self, + id: &str, + category: &str, + description: &str, + frequency: i64, + trajectory_refs_json: &str, + disposition: &str, + developer_notes: Option<&str>, + ) -> Result; + + async fn list_feature_requests( + &self, + disposition: Option<&str>, + ) -> Result, PersistenceError>; + + async fn update_feature_request( + &self, + id: &str, + disposition: &str, + developer_notes: Option<&str>, + ) -> Result; + + async fn insert_evolution_record( + &self, + id: &str, + record_type: &str, + status: &str, + created_by: &str, + derived_from: Option<&str>, + data_json: &str, + ) -> Result<(), PersistenceError>; + + async fn get_evolution_record( + &self, + id: &str, + ) -> Result, PersistenceError>; + + async fn list_evolution_records( + &self, + record_type: Option<&str>, + status: Option<&str>, + ) -> Result, PersistenceError>; + + async fn list_ranked_insights(&self) -> Result, PersistenceError>; +} + +/// Design-time verification event capability. +#[async_trait::async_trait] +pub trait DesignTimeEventStore: Send + Sync { + #[allow(clippy::too_many_arguments)] + async fn insert_design_time_event( + &self, + kind: &str, + entity_type: &str, + tenant: &str, + summary: &str, + level: Option<&str>, + passed: Option, + step_number: Option, + total_steps: Option, + ) -> Result<(), PersistenceError>; + + async fn list_design_time_events( + &self, + tenant: Option<&str>, + limit: i64, + ) -> Result, PersistenceError>; +} + +/// OTS trajectory capability. +#[async_trait::async_trait] +pub trait OtsStore: Send + Sync { + async fn persist_ots_trajectory( + &self, + params: &OtsTrajectoryParams<'_>, + ) -> Result<(), PersistenceError>; + + async fn enqueue_ots_trajectory( + &self, + params: &OtsTrajectoryParams<'_>, + ) -> Result<(), PersistenceError>; + + async fn mark_ots_trajectory_persisted( + &self, + trajectory_id: &str, + ) -> Result<(), PersistenceError>; + + async fn mark_ots_trajectory_failed( + &self, + trajectory_id: &str, + error: &str, + ) -> Result<(), PersistenceError>; + + async fn list_queued_ots_trajectories( + &self, + limit: i64, + ) -> Result, PersistenceError>; + + async fn list_ots_trajectories( + &self, + tenant: &str, + agent_id: Option<&str>, + outcome: Option<&str>, + limit: i64, + ) -> Result, PersistenceError>; + + async fn get_ots_trajectory( + &self, + trajectory_id: &str, + ) -> Result, PersistenceError>; +} diff --git a/crates/temper-server/src/storage/mod.rs b/crates/temper-server/src/storage/mod.rs index f689b7db0..03b6e9c92 100644 --- a/crates/temper-server/src/storage/mod.rs +++ b/crates/temper-server/src/storage/mod.rs @@ -34,6 +34,7 @@ use crate::platform_store::PlatformStore; use crate::platform_store::SimPlatformStore; use crate::state::trajectory::{TrajectoryEntry, TrajectorySource}; +mod capabilities; mod published_artifacts; mod query_plane_impls; mod query_plane_read; @@ -861,122 +862,7 @@ pub trait ObserveReadStore: Send + Sync { ) -> Result, PersistenceError>; } -/// Evolution engine durable metadata capability. -#[async_trait::async_trait] -pub trait EvolutionStore: Send + Sync { - #[allow(clippy::too_many_arguments)] - async fn upsert_feature_request( - &self, - id: &str, - category: &str, - description: &str, - frequency: i64, - trajectory_refs_json: &str, - disposition: &str, - developer_notes: Option<&str>, - ) -> Result<(), PersistenceError>; - - async fn list_feature_requests( - &self, - disposition: Option<&str>, - ) -> Result, PersistenceError>; - - async fn update_feature_request( - &self, - id: &str, - disposition: &str, - developer_notes: Option<&str>, - ) -> Result; - - async fn insert_evolution_record( - &self, - id: &str, - record_type: &str, - status: &str, - created_by: &str, - derived_from: Option<&str>, - data_json: &str, - ) -> Result<(), PersistenceError>; - - async fn get_evolution_record( - &self, - id: &str, - ) -> Result, PersistenceError>; - - async fn list_evolution_records( - &self, - record_type: Option<&str>, - status: Option<&str>, - ) -> Result, PersistenceError>; - - async fn list_ranked_insights(&self) -> Result, PersistenceError>; -} - -/// Design-time verification event capability. -#[async_trait::async_trait] -pub trait DesignTimeEventStore: Send + Sync { - #[allow(clippy::too_many_arguments)] - async fn insert_design_time_event( - &self, - kind: &str, - entity_type: &str, - tenant: &str, - summary: &str, - level: Option<&str>, - passed: Option, - step_number: Option, - total_steps: Option, - ) -> Result<(), PersistenceError>; - - async fn list_design_time_events( - &self, - tenant: Option<&str>, - limit: i64, - ) -> Result, PersistenceError>; -} - -/// OTS trajectory capability. -#[async_trait::async_trait] -pub trait OtsStore: Send + Sync { - async fn persist_ots_trajectory( - &self, - params: &OtsTrajectoryParams<'_>, - ) -> Result<(), PersistenceError>; - - async fn enqueue_ots_trajectory( - &self, - params: &OtsTrajectoryParams<'_>, - ) -> Result<(), PersistenceError>; - - async fn mark_ots_trajectory_persisted( - &self, - trajectory_id: &str, - ) -> Result<(), PersistenceError>; - - async fn mark_ots_trajectory_failed( - &self, - trajectory_id: &str, - error: &str, - ) -> Result<(), PersistenceError>; - - async fn list_queued_ots_trajectories( - &self, - limit: i64, - ) -> Result, PersistenceError>; - - async fn list_ots_trajectories( - &self, - tenant: &str, - agent_id: Option<&str>, - outcome: Option<&str>, - limit: i64, - ) -> Result, PersistenceError>; - - async fn get_ots_trajectory( - &self, - trajectory_id: &str, - ) -> Result, PersistenceError>; -} +pub use capabilities::{DesignTimeEventStore, EvolutionStore, OtsStore}; /// Legacy database-backed blob capability. #[async_trait::async_trait] @@ -1867,7 +1753,7 @@ impl EvolutionStore for PostgresEventStore { trajectory_refs_json: &str, disposition: &str, developer_notes: Option<&str>, - ) -> Result<(), PersistenceError> { + ) -> Result { self.upsert_feature_request( id, category, @@ -1949,7 +1835,7 @@ impl EvolutionStore for TursoEventStore { trajectory_refs_json: &str, disposition: &str, developer_notes: Option<&str>, - ) -> Result<(), PersistenceError> { + ) -> Result { self.upsert_feature_request( id, category, diff --git a/crates/temper-store-postgres/src/platform.rs b/crates/temper-store-postgres/src/platform.rs index 322e61fe9..caed9e178 100644 --- a/crates/temper-store-postgres/src/platform.rs +++ b/crates/temper-store-postgres/src/platform.rs @@ -1785,28 +1785,45 @@ impl PostgresEventStore { trajectory_refs_json: &str, disposition: &str, developer_notes: Option<&str>, - ) -> Result<(), PersistenceError> { + ) -> Result { let trajectory_refs = parse_json(trajectory_refs_json)?; - crate::dbm::postgres_query!( + // ARN-240: disposition and developer_notes are developer-owned after + // creation — regeneration writes them ONLY on first insert, and the + // caller learns whether this insert created the record. + let result = crate::dbm::postgres_query!( "INSERT INTO feature_requests \ (id, category, description, frequency, trajectory_refs, disposition, developer_notes, updated_at) \ VALUES ($1, $2, $3, $4, $5, $6, $7, now()) \ - ON CONFLICT (id) DO UPDATE SET \ - category = EXCLUDED.category, description = EXCLUDED.description, frequency = EXCLUDED.frequency, \ - trajectory_refs = EXCLUDED.trajectory_refs, disposition = EXCLUDED.disposition, \ - developer_notes = EXCLUDED.developer_notes, updated_at = now()", + ON CONFLICT (id) DO NOTHING", ) .bind(id) .bind(category) .bind(description) .bind(frequency) - .bind(trajectory_refs) + .bind(trajectory_refs.clone()) .bind(disposition) .bind(developer_notes) .execute(self.pool()) .await .map_err(storage_error)?; - Ok(()) + if result.rows_affected() > 0 { + return Ok(true); + } + crate::dbm::postgres_query!( + "UPDATE feature_requests SET \ + category = $2, description = $3, frequency = $4, \ + trajectory_refs = $5, updated_at = now() \ + WHERE id = $1", + ) + .bind(id) + .bind(category) + .bind(description) + .bind(frequency) + .bind(trajectory_refs) + .execute(self.pool()) + .await + .map_err(storage_error)?; + Ok(false) } pub async fn list_feature_requests( diff --git a/crates/temper-store-turso/src/store/evolution.rs b/crates/temper-store-turso/src/store/evolution.rs index ee220eb1c..170389916 100644 --- a/crates/temper-store-turso/src/store/evolution.rs +++ b/crates/temper-store-turso/src/store/evolution.rs @@ -24,20 +24,34 @@ impl TursoEventStore { trajectory_refs_json: &str, disposition: &str, developer_notes: Option<&str>, - ) -> Result<(), PersistenceError> { + ) -> Result { let _query_timer = TursoQueryTimer::start("turso.upsert_feature_request"); let conn = self.configured_connection().await?; + // ARN-240: disposition and developer_notes are developer-owned after + // creation — regeneration writes them ONLY on first insert, and the + // caller learns whether this insert created the record. + let inserted = conn + .execute( + "INSERT INTO feature_requests (id, category, description, frequency, trajectory_refs, disposition, developer_notes, updated_at) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, datetime('now')) \ + ON CONFLICT(id) DO NOTHING", + params![id, category, description, frequency, trajectory_refs_json, disposition, developer_notes], + ) + .await + .map_err(storage_error)?; + if inserted > 0 { + return Ok(true); + } conn.execute( - "INSERT INTO feature_requests (id, category, description, frequency, trajectory_refs, disposition, developer_notes, updated_at) \ - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, datetime('now')) \ - ON CONFLICT(id) DO UPDATE SET \ + "UPDATE feature_requests SET \ category = ?2, description = ?3, frequency = ?4, trajectory_refs = ?5, \ - disposition = ?6, developer_notes = ?7, updated_at = datetime('now')", - params![id, category, description, frequency, trajectory_refs_json, disposition, developer_notes], + updated_at = datetime('now') \ + WHERE id = ?1", + params![id, category, description, frequency, trajectory_refs_json], ) .await .map_err(storage_error)?; - Ok(()) + Ok(false) } /// List feature requests with optional disposition filter. diff --git a/docs/adrs/0163-feature-request-read-idempotency.md b/docs/adrs/0163-feature-request-read-idempotency.md new file mode 100644 index 000000000..3a87e2ebd --- /dev/null +++ b/docs/adrs/0163-feature-request-read-idempotency.md @@ -0,0 +1,70 @@ +# ADR-0163: Feature-Request Reads Are Idempotent + +## Status + +Accepted (2026-07-13) + +(Numbered 0163: 0156–0162 are claimed by concurrently open arena branches.) + +## Context + +`GET /observe/evolution/feature-requests` regenerates feature requests from +trajectory gap analysis on every read and persists what it generates. Every +generated record minted a fresh UUID-suffixed id (`RecordHeader::new`), so +the store "upsert" — keyed on that id — inserted a NEW row on every GET, and +a fresh `FR-{uuid}` system entity was dispatched per generated record per +GET. Reads spawned unbounded duplicates (ARN-240). The upsert also +overwrote `disposition` and `developer_notes` with generator defaults, so a +developer's WontFix could be silently reset by any read racing a re-listing. + +## Decision + +1. **Identity is derived from content, not minted.** A feature request's id + is a stable hash of its gap group key — `FR-{sha256(action, error_pattern) + [..12]}`. The same gap always maps to the same record, making generation + idempotent by construction rather than by bookkeeping. +2. **Insert and update are separated, and developer-owned fields are only + ever written on insert.** `upsert_feature_request` first attempts + `INSERT … ON CONFLICT (id) DO NOTHING`; if the row already existed, an + UPDATE refreshes only the generator-owned fields (category, description, + frequency, trajectory_refs, updated_at). `disposition` and + `developer_notes` belong to the developer after creation and are never + touched by re-generation. The method returns whether it inserted. +3. **System entities are created once, keyed by the record id.** The handler + dispatches `CreateFeatureRequest` only when the store reports a fresh + insert, and the entity id IS the deterministic record id (the previous + code minted a second, unrelated `FR-{uuid}` per GET). + +## Consequences + +- A GET is now a read: repeating it changes nothing. The regeneration that + runs inside it converges on the same rows and skips entity creation for + anything already known. +- Existing duplicate rows from the previous behavior are not migrated — + they remain until dispositioned. (A cleanup migration was considered and + rejected: rows may carry developer notes; deletion is a human decision.) +- Two concurrent GETs race benignly: both compute the same id, one wins the + insert, the other's `DO NOTHING` reports "existed" and skips the entity. +- **Entity dispatch is at-most-once with no reconciliation.** If the process + dies between the successful insert and the entity dispatch — or the + dispatch fails (it is warn-only) — the system entity for that record is + never created: every later GET sees an existing row and skips. The + previous behavior was strictly worse (a new entity per read), so this is + accepted here; a reconciliation sweep (create entities for rows whose + journal is empty) is the follow-up if the entity plane becomes + load-bearing. +- The frequency/description of an existing record now tracks the latest + generation window rather than accumulating forever — which is what the + listing already claimed to show. + +## Alternatives Considered + +- **Moving generation out of the GET entirely** (event-driven, e.g. on + trajectory write or sentinel schedule): the cleaner long-term shape — + reads should not write at all — but it changes when insights appear and + belongs with the broader evolution-engine scheduling work. The + deterministic identity fix is required under any scheduling model, and + once it is in, the in-GET generation is harmless (idempotent). +- **Deduplicating in the generator against existing rows** (read-modify- + write): racy without a transaction spanning generation, and still leaves + identity random — the class survives anywhere a second writer appears. From 904e9af88319a856915668388ba938d454acfb76 Mon Sep 17 00:00:00 2001 From: Rita Agafonova <36133358+rita-aga@users.noreply.github.com> Date: Tue, 14 Jul 2026 15:08:39 -0700 Subject: [PATCH 3/3] docs(adr): record the tenant-scope and legacy_record_id consequences (ARN-240) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile's two P2s, both accepted residuals rather than defects: the gap-group key excludes tenant (pre-existing grouping semantics the deterministic id cements — tenant-scoped grouping is a separate product decision), and legacy_record_id now equals the entity id at this dispatch site (kept for uniformity with the other five evolution dispatch sites). Co-Authored-By: Claude Fable 5 --- docs/adrs/0163-feature-request-read-idempotency.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/adrs/0163-feature-request-read-idempotency.md b/docs/adrs/0163-feature-request-read-idempotency.md index 3a87e2ebd..23f992f10 100644 --- a/docs/adrs/0163-feature-request-read-idempotency.md +++ b/docs/adrs/0163-feature-request-read-idempotency.md @@ -56,6 +56,15 @@ developer's WontFix could be silently reset by any read racing a re-listing. - The frequency/description of an existing record now tracks the latest generation window rather than accumulating forever — which is what the listing already claimed to show. +- **The gap-group key excludes tenant** — identical gaps across tenants share + one record. This is pre-existing grouping semantics (the accumulation map + always merged across tenants); the deterministic id cements it rather than + introducing it. Tenant-scoped grouping is a deliberate product decision to + take separately, not a side effect to smuggle into a duplication fix. +- `legacy_record_id` in the entity dispatch params now equals the entity id + at this site (they were distinct when entity ids were minted per read). + The key is kept: it is the uniform convention across all six evolution + dispatch sites, and consumers read it generically. ## Alternatives Considered