Skip to content

ARN-179: execute PostgreSQL actor effects durably - #372

Closed
nerdsane wants to merge 13 commits into
mainfrom
codex/arn-179-effect-vocabulary
Closed

ARN-179: execute PostgreSQL actor effects durably#372
nerdsane wants to merge 13 commits into
mainfrom
codex/arn-179-effect-vocabulary

Conversation

@nerdsane

@nerdsane nerdsane commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Behavioral / durability regression\n\nPostgreSQL-backed actors could accept a transition while silently dropping runtime effects. The durable actor path interpreted only a subset of the canonical effect vocabulary, so accepted actions could lose field mutations, item updates, counter changes, tells, schedules, or spawns across commit/restart boundaries.\n\nThis is a non-security correctness and durability fix for ARN-179.\n\n## RED\n\n- Added a PostgreSQL actor regression that drives the complete effect vocabulary through the durable runtime.\n- Confirmed the test failed before the fix because effects were not fully executed/persisted.\n- RED commit: 766ad94\n\n## GREEN\n\n- Added one exhaustive canonical effect executor in temper-jit and shared it across in-memory/server and PostgreSQL actor adapters.\n- Made tells, schedules, and spawns transaction-buffered so rollback cannot leak side effects.\n- Added durable delayed-message staging and globally ordered promotion.\n- Made PostgreSQL actor schema initialization atomic and safe under concurrent startup.\n- Preserved full compatible reaction registries; rejected reaction semantics that the CLI actor mode cannot preserve instead of silently degrading them.\n- GREEN commit: 2209723\n\n## Validation\n\n- Dedicated local code-quality review: PASS\n- Dedicated DST compliance review: PASS\n- PostgreSQL durability E2E: 4/4\n- Canonical effect conformance: PASS\n- CLI reaction wiring: 13/13\n- Server effect tests: 27/27\n- Actor runtime spec tests: 5/5\n- Full workspace tests and doctests: PASS\n- Pre-push rustfmt, strict clippy, readability ratchet, and full workspace suite: ALL GATES PASSED\n\nExact live commands and output are posted as a PR comment.

Greptile Summary

This PR fixes a correctness and durability regression in the PostgreSQL-backed actor runtime: the durable activation path previously interpreted only a subset of the effect vocabulary, silently dropping field mutations, counter changes, list operations, tells, schedules, and spawns across commit/restart boundaries.

  • Canonical effect executor in temper-jit: A single exhaustive apply_effects function now covers the full Effect enum and is shared across the in-memory, server, and PostgreSQL backends — eliminating the silent-discard behaviour and making simulation and production code paths identical.
  • Transaction-buffered spawns and durable delayed messages: Child actor creations and scheduled tells are now buffered with the parent activation transaction; rollback atomically retracts state mutations, queued tells, staged timers, and child actors. The CREATE_ACTOR ON CONFLICT DO NOTHING idempotency fix ensures a duplicate spawn does not re-deliver the initial action.
  • Schema initialization and scheduled-message promotion: create_tables now holds a pg_advisory_xact_lock over the full DDL sequence, and the PROMOTE_DUE_MESSAGES CTE atomically selects, locks, deletes, and reinserts due messages with a global advisory lock, preserving FIFO order under concurrent promoters.

Confidence Score: 5/5

Safe to merge — the change restores correct, atomic execution of the full effect vocabulary for PostgreSQL-backed actors with no regressions found in the shared canonical executor or the transaction flush paths.

All durable paths (spawn, scheduled tell, immediate tell, state mutation) are now committed or rolled back together inside one PostgreSQL transaction. The canonical effect executor is exhaustive by design — adding a new Effect variant fails to compile until its semantics are defined. The idempotent-spawn guard, cascade-depth budget, advisory-lock schema initialization, and ordered timer promotion are each covered by dedicated PostgreSQL integration tests. No logic gaps or unguarded edge cases were found across the changed files.

No files require special attention. The most complex new logic — the budget calculation in spec_actor.rs and the spawn-flush loop in pg.rs — has direct test coverage in effect_durability.rs.

Important Files Changed

Filename Overview
crates/temper-jit/src/table/effects.rs New canonical effect executor: exhaustive match over Effect enum, shared across all backends. Logic for counters, booleans, lists, spawn, schedule, and schedule_at is sound.
crates/temper-actor-runtime/src/pg.rs Adds transaction-buffered spawn flush and durable scheduled-tell routing; created==1 guard correctly prevents duplicate initial messages for idempotent child creation.
crates/temper-actor-runtime/src/schema.rs Adds actor_scheduled_messages DDL, advisory-lock-protected create_tables, INSERT_SCHEDULED_MESSAGE, and PROMOTE_DUE_MESSAGES CTE. Lock scope and ordering semantics are correctly documented and verified by the concurrent-promoter test.
crates/temper-actor-runtime/src/spec_actor.rs Replaces per-actor routing HashMap with canonical ReactionRegistry; implements EffectState for SpecActorState; adds cascade_depth tracking; delegates apply_effects to the shared JIT executor. Budget computation is conservative and correct.
crates/temper-actor-runtime/src/spec_actor/commands.rs Implements execute_commands and reaction routing with cascade-depth enforcement; resolve_reaction_target correctly handles all TargetResolver variants; CreateIfMissing buffers spawn atomically with the parent activation.
crates/temper-actor-runtime/tests/effect_durability.rs Comprehensive PostgreSQL regression: covers list-append persistence, guarded-action after restart, spawn (idempotent + initial-action-once), timer staging and promotion, rollback of all side effects on failure, and concurrent schema initialisation.
crates/temper-actor-runtime/tests/reaction_durability.rs PostgreSQL regressions for timer field staleness after restart and reaction-cycle depth enforcement across commit/restart boundaries.
crates/temper-runtime/src/reaction.rs Adds params/params_from to ReactionTarget, switches index key to typed BTreeMap tuple, adds From convenience, and adds budget-calculation helper methods.
crates/temper-cli/src/serve/actor_runtime.rs Replaces permissive effect allowlist with full reaction-rule wiring; rejects guarded/principal/Create resolver semantics at startup. Conflict check extended to reaction rule equality across tenants.
crates/temper-agents/specs/process.reactions.toml Adds params_from mappings to all process reactions; splits notify_parent into three state-specific rules (Terminated/Ready/Failed).

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Handler as SpecDrivenActor.handle()
    participant JIT as temper_jit::apply_effects()
    participant Ctx as ActorContext (buffers)
    participant PG as PostgreSQL tx

    Handler->>JIT: "apply_effects(&mut state, &effects, &params)"
    JIT-->>Handler: "EffectExecution {emitted_events, scheduled_actions, spawn_requests}"
    Handler->>Ctx: execute_commands(execution, cascade_depth)
    Ctx->>Ctx: ctx.tell(target, msg) to pending_tells
    Ctx->>Ctx: ctx.tell_after(self, msg, delay) to pending_tells (deliver_at set)
    Ctx->>Ctx: ctx.buffer_spawn(target, fields, init_msg) to pending_spawns
    Handler->>PG: UPDATE actor_instances SET state, version
    PG-->>Handler: affected rows (optimistic lock check)
    Handler->>PG: INSERT actor_scheduled_messages (deliver_at tells)
    Handler->>PG: INSERT actor_messages (immediate tells)
    Handler->>PG: INSERT actor_instances (spawned children, ON CONFLICT DO NOTHING)
    Note over Handler,PG: INSERT initial_message only when created==1
    Handler->>PG: COMMIT
    Note over PG: Scheduler PROMOTE_DUE_MESSAGES CTE moves due timers into actor_messages FIFO
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Handler as SpecDrivenActor.handle()
    participant JIT as temper_jit::apply_effects()
    participant Ctx as ActorContext (buffers)
    participant PG as PostgreSQL tx

    Handler->>JIT: "apply_effects(&mut state, &effects, &params)"
    JIT-->>Handler: "EffectExecution {emitted_events, scheduled_actions, spawn_requests}"
    Handler->>Ctx: execute_commands(execution, cascade_depth)
    Ctx->>Ctx: ctx.tell(target, msg) to pending_tells
    Ctx->>Ctx: ctx.tell_after(self, msg, delay) to pending_tells (deliver_at set)
    Ctx->>Ctx: ctx.buffer_spawn(target, fields, init_msg) to pending_spawns
    Handler->>PG: UPDATE actor_instances SET state, version
    PG-->>Handler: affected rows (optimistic lock check)
    Handler->>PG: INSERT actor_scheduled_messages (deliver_at tells)
    Handler->>PG: INSERT actor_messages (immediate tells)
    Handler->>PG: INSERT actor_instances (spawned children, ON CONFLICT DO NOTHING)
    Note over Handler,PG: INSERT initial_message only when created==1
    Handler->>PG: COMMIT
    Note over PG: Scheduler PROMOTE_DUE_MESSAGES CTE moves due timers into actor_messages FIFO
Loading

Reviews (4): Last reviewed commit: "fix: initialize durable child actors onc..." | Re-trigger Greptile

@nerdsane

Copy link
Copy Markdown
Owner Author

Live local PostgreSQL E2E evidence

This is a non-security behavioral/durability regression suite. It exercises the previously lossy durable actor path against PostgreSQL 17, including restart persistence, transaction rollback, concurrent timer promotion ordering, and concurrent schema initialization.

$ TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' cargo test -p temper-actor-runtime --test effect_durability -- --nocapture
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.31s
     Running tests/effect_durability.rs (target/debug/deps/effect_durability-9ef044f435447ea3)

running 4 tests
test command_flush_failure_rolls_back_parent_state_cursor_and_commands ... ok
test concurrent_schema_initializers_wait_for_the_global_ddl_lock ... ok
test concurrent_timer_promoters_preserve_global_due_order ... ok
test postgres_effects_survive_restart_and_commit_runtime_commands ... ok

test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.14s

Full local gate:

$ RUST_TEST_THREADS=1 TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' cargo test --workspace
...
ARN179_WORKSPACE_SERIAL_EXIT:0

Push hook:

Gate 1/4: rustfmt check
Gate 2/4: cargo clippy
Gate 3/4: Readability ratchet
Gate 4/4: Full test suite
Pre-push: ALL GATES PASSED

@nerdsane

Copy link
Copy Markdown
Owner Author

Dedicated GPT-5.6 review of the open GitHub diff at head 2209723.

The exhaustive JIT match, atomic PostgreSQL state/command flush, scheduled-message staging, serialized promotion query, and transactional DDL initialization are directionally sound. The following behavioral and durability regressions remain blocking:

  1. [P1] Scheduled delivery can overwrite newer durable fields with a stale snapshot — crates/temper-actor-runtime/src/spec_actor/commands.rs:31-43 and crates/temper-actor-runtime/src/spec_actor.rs:282-296. Both relative and absolute timers serialize state.fields at scheduling time. When the timer is later consumed, SpecDrivenActor merges that old snapshot into the then-current state before evaluating the action. Any field changed after scheduling is silently reverted when the timer fires, including after restart. The server path sends only scheduling metadata, not a state snapshot. Queue empty or reserved scheduling params instead, and add a PostgreSQL regression that schedules, commits an intervening field update, restarts, promotes the timer, and proves the newer field survives.

  2. [P1] Reactions copy undeclared source fields into the target — crates/temper-actor-runtime/src/spec_actor/commands.rs:103-116 and crates/temper-cli/src/serve/actor_runtime.rs:305-314. Startup explicitly accepts only reactions whose params and params_from are empty, but route_rules sends the complete source state.fields as target action params. The target actor then merges those values into its own durable fields, so a valid empty-parameter reaction can overwrite unrelated target data and differs from the server dispatcher, which sends the declared empty parameter object. Carry the effective declared reaction params through the actor rule or send an empty object for the currently supported subset, with a regression using conflicting source and target field names.

  3. [P1] Field and CreateIfMissing resolvers are accepted as lossless but change commit behavior — crates/temper-actor-runtime/src/spec_actor/commands.rs:128-143 and crates/temper-cli/src/serve/actor_runtime.rs:316-325. The actor adapter turns a missing or empty Field target into HandlerFailed, rolling back the source transition and retrying the same mailbox message. The canonical server reaction path records that reaction as unresolved while preserving the already successful source transition. For CreateIfMissing, the server derives source-id-derived when id_field is absent, while this adapter also fails the source activation. Share the canonical resolver semantics or reject these variants at startup until they can be preserved; cover missing and empty fields in durable activation tests.

  4. [P2] Reaction cascades have no durable depth budget — crates/temper-runtime/src/reaction.rs:24-25 and crates/temper-actor-runtime/src/spec_actor/commands.rs:73-119. MAX_REACTION_DEPTH remains part of the reaction contract, but SpecMessage carries no cascade depth and every routed action starts a fresh activation. A valid two-actor cyclic reaction therefore appends messages forever instead of stopping at the canonical depth budget. Persist and increment reaction depth in routed messages, or reject cyclic actor-runtime reaction graphs at startup, and add a bounded two-actor cycle regression across scheduler polls and restart.

  5. [P2] Valid command fan-out can panic at the new buffer boundary — crates/temper-actor-runtime/src/actor.rs:203-249. The reaction registry permits 256 rules, exactly the tell buffer capacity. One automatically emitted action plus an explicit emit or schedule can legitimately produce more than 256 commands, at which point tell or tell_at asserts rather than returning an activation error. In the scheduler path that panic can also bypass normal in-flight removal. Align admission and execution budgets, return a handled error instead of panicking on accepted specs, and test the maximum allowed reaction fan-out combined with another command effect.

Verdict: FAIL

@nerdsane

Copy link
Copy Markdown
Owner Author

Reviewer-finding RED/GREEN evidence

The dedicated GPT-5.6 review at head 22097239 returned Verdict: FAIL with five behavioral/durability blockers. They were reproduced in a RED-only commit and fixed in a separate GREEN commit:

  • RED: dfee8cfc test: reproduce reaction command durability gaps
  • GREEN: 8246c795 fix: preserve reaction command durability

RED (before fix)

reaction unit regressions: 4 failed, 1 passed
- source fields copied into empty-parameter reaction payloads
- unresolved Field target rolled back the source transition
- CreateIfMissing failed instead of deriving the canonical ID
- accepted fan-out + schedule panicked at the tell-buffer boundary

PostgreSQL restart regressions: 0 passed, 2 failed
- timer field: left "old", right "new"
- cyclic reaction activations: left 36, right 9

GREEN (current head)

$ TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' cargo test -p temper-actor-runtime

unit tests:                 10 passed; 0 failed
effect_durability:           4 passed; 0 failed
integration:                 5 passed; 0 failed
reaction_durability:         2 passed; 0 failed

Additional behavioral contracts now green:

  • Scheduled messages carry action intent only; newer durable fields survive promotion and restart.
  • Empty-parameter reactions do not inject source fields into target state.
  • Missing/empty Field targets are skipped without rolling back the successful source action.
  • CreateIfMissing derives <source-id>-derived when its field is absent.
  • Protobuf-persisted cascade depth stops a two-actor cycle at depth 8 across restart.
  • Handler-derived finite-table budgets admit 256 wildcard CreateIfMissing rules plus automatic action emit, explicit emit, and schedule: 513 tells and 512 spawns.
  • True buffer overflow returns ActorError::HandlerFailed; it does not panic or mutate the buffer.
  • Typed (actor_type, Option<action>) registry keys prevent literal-* duplication and colon-delimiter aliasing.
$ cargo test -p temper-runtime reaction
test result: ok. 6 passed; 0 failed

$ cargo clippy --workspace --all-targets -- -D warnings
Finished `dev` profile

$ RUST_TEST_THREADS=1 TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' cargo test --workspace
...
ARN179_REVIEW_FIX_WORKSPACE_EXIT:0

Updated push hook:

Gate 1/4: rustfmt check
Gate 2/4: cargo clippy
Gate 3/4: Readability ratchet
Gate 4/4: Full test suite
Pre-push: ALL GATES PASSED
ARN179_REVIEW_FIX_PUSH_EXIT:0

@nerdsane

Copy link
Copy Markdown
Owner Author

Dedicated GPT-5.6 re-review of the open GitHub PR diff at head 8246c79518f1dbd40a81b1a7f608133b0acb1f05.

I rechecked all five blockers from the prior Verdict: FAIL comment and found each resolved:

  1. Scheduled-state durability: relative and absolute timers now serialize only SpecMessage::new(action), so a promoted timer cannot merge a stale scheduling-time field snapshot into newer durable state. scheduled_action_does_not_restore_stale_fields_after_restart proves the intervening update survives restart and timer promotion.
  2. Reaction parameter isolation: routed reactions now use an empty parameter payload for the adapter-supported empty-parameter subset. Source fields are no longer copied into target state, and startup still rejects richer parameter-mapping semantics that cannot be preserved.
  3. Resolver commit semantics: missing, empty, or non-string Field targets return None and skip only that reaction while preserving the successful source transition. CreateIfMissing now uses the explicit non-empty field when present and otherwise derives <source-id>-derived, matching the canonical server resolver.
  4. Durable cascade bound: protobuf tag 3 adds cascade_depth compatibly; legacy payloads decode the absent scalar as zero. Direct routed messages persist depth + 1, scheduled and external actions begin at zero, and routing stops at MAX_REACTION_DEPTH. The two-actor PostgreSQL test restarts halfway through the cycle and drains at exactly the bounded activation count.
  5. Admitted fan-out boundary: SpecDrivenActor derives tell and spawn budgets from every finite transition rule and the complete reaction registry. The count includes the automatically appended action emit, explicit emit/custom effects, schedules, explicit spawns, and every CreateIfMissing spawn. The maximum accepted 256-rule wildcard/CreateIfMissing fan-out plus emit and schedule produces 513 tells and 512 spawns without panic. Actual overflow is fallible and leaves the buffer unchanged.

Additional review:

  • ReactionRegistry now keys by (String, Option<String>); a literal * action no longer aliases the wildcard key, and actor/action names containing : no longer collide. Its per-actor rule counts traverse the typed entries once and conservatively bound every possible match.
  • tell, tell_at, tell_after, and reply return Result<(), ActorError>. Production downstream call sites propagate these errors, so command-budget exhaustion rolls the activation transaction back through the normal handler-error path.
  • The canonical JIT effect executor remains the shared exhaustive state/effect path for the server and PostgreSQL actor adapter. The current diff adds no nondeterministic pattern to temper-runtime, temper-jit, or temper-server; the integrity/DST pattern GitHub check is green.
  • Reviewer validation passed: cargo test -p temper-runtime reaction (6/6), cargo test -p temper-actor-runtime spec_actor::tests::reactions (6/6), and PostgreSQL cargo test -p temper-actor-runtime --test reaction_durability -- --nocapture (2/2). git diff --check origin/main...8246c795 is clean. Compile/lint, spec verification, instrumentation hygiene, platform-random DST, and integrity/DST checks are green; remaining GitHub jobs were still pending at review time and do not change this code-review verdict.

No actionable correctness or durability finding remains in the current open PR diff.

Verdict: PASS

@nerdsane

Copy link
Copy Markdown
Owner Author

@greptile review

Comment thread crates/temper-actor-runtime/src/spec_actor/commands.rs
Comment thread crates/temper-actor-runtime/src/spec_actor/commands.rs Outdated
Comment thread crates/temper-actor-runtime/src/spec_actor/commands.rs
Comment thread crates/temper-actor-runtime/src/schema.rs
@nerdsane

Copy link
Copy Markdown
Owner Author

Final Greptile-finding RED/GREEN and live E2E evidence

The four prior Greptile threads were treated as non-security behavioral/durability regressions. The two executable failures were isolated before the fixes:

  • RED 1b913854 test: reproduce actor reaction review regressions
    • declared reaction parameters arrived as an empty payload;
    • a valid nine-spawn transition was rejected by the obsolete fixed eight-spawn ceiling.
  • GREEN ac486be7 fix: preserve declared actor reaction parameters
    • actor reactions now preserve explicit static params and deterministic params_from mappings with the same missing-field, Id, scalar, and collision behavior as the server;
    • Temper Agents declares only the fields each production integration consumes;
    • direct spawns rely on the finite handler-derived activation budget;
    • spawn initial messages derive their stored type from the Rust message type;
    • the timer promotion SQL documents that its transaction advisory lock depends on the complete CTE remaining one statement.

RED evidence:

declared_reaction_params_copy_only_named_source_fields ... FAILED
  routed params must be JSON: EOF while parsing a value

admitted_direct_spawns_use_the_derived_activation_budget ... FAILED
  HandlerFailed("spawn effect budget exceeded (8 per transition)")

Live local PostgreSQL production-chain E2E after GREEN:

$ TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' \
    cargo test -p temper-agents --test agent_chain \
    test_process_reaction_forwards_only_declared_prompt_to_llm -- --nocapture

running 1 test
test test_process_reaction_forwards_only_declared_prompt_to_llm ... ok
test result: ok. 1 passed; 0 failed

That test persists the actual LlmIntegration actor state after the Process → ContextManager → Process → LlmIntegration chain and proves it is exactly:

{"user_prompt":"declared prompt"}

The undeclared private_source_field is absent.

Final local gates at head ac486be7:

cargo fmt --all -- --check                                      PASS
git diff --check origin/main...HEAD                              PASS
scripts/readability-ratchet.sh check .ci/readability-baseline.env PASS
cargo clippy --workspace --all-targets -- -D warnings            PASS
RUST_TEST_THREADS=1 TEMPER_TEST_DATABASE_URL=... cargo test --workspace PASS
mandatory code review: PASS, 0 findings
mandatory DST review: PASS, 0 findings, architecture CLEAN

Normal git push hook independently reran and passed all four gates:

Gate 1/4: rustfmt check
Gate 2/4: cargo clippy
Gate 3/4: Readability ratchet
Gate 4/4: Full test suite
Pre-push: ALL GATES PASSED

@nerdsane

Copy link
Copy Markdown
Owner Author

Dedicated GPT-5.6 review of the open GitHub PR diff at exact head ac486be750d8cb8b0dddcc669dd20aa85d0fc01d.

I rechecked the full origin/main...ac486be7 change set and the PR conversation. The four prior Greptile findings are resolved at this head: spawn initial messages derive their stored type from SpecMessage; direct spawns use the handler-derived budget; actor reactions now carry explicit params/params_from mappings and the Temper Agents production rules declare their consumed fields; and PROMOTE_DUE_MESSAGES documents the single-statement transaction-lock requirement. Focused actor reaction tests pass 8/8 and git diff --check is clean.

One non-security correctness/durability blocker remains:

  1. [P1] Non-object static reaction params overwrite the target actor's complete durable field state. crates/temper-cli/src/serve/actor_runtime.rs:305-340 now accepts every server then.params JSON shape and passes it into the PostgreSQL actor runtime. crates/temper-actor-runtime/src/spec_actor/commands.rs:183-199 deliberately forwards a scalar or array unchanged, but crates/temper-actor-runtime/src/spec_actor.rs:304-323 handles any non-empty, non-object payload by assigning actor_state.fields = action_params. A valid reaction such as params = "tick" therefore replaces an existing target object such as {"account_id":"a-1","balance":10} with the string "tick" when the mailbox message commits. That is not the canonical server behavior: build_effective_params preserves non-object params as action input, while sync_fields_with_metadata projects params only when they are an object (crates/temper-server/src/entity_actor/effects/fields.rs:50-76), leaving existing durable fields intact. The newly enabled CLI conversion thus claims lossless support for a server-valid reaction shape but silently destroys unrelated target state. Preserve scalar/array action input without projecting it into SpecActorState.fields, or reject those shapes at actor-runtime startup, and add a restart-backed regression proving pre-existing target fields survive.

Verdict: FAIL

@nerdsane

Copy link
Copy Markdown
Owner Author

Dedicated reviewer FAIL follow-up — RED/GREEN

The exact-head GPT-5.6 review at ac486be7 correctly returned Verdict: FAIL because scalar/array reaction parameters could replace the actor's complete durable fields value. That was a behavioral/durability regression: an otherwise successful transition could discard unrelated persisted fields.

Reviewer verdict: #372 (comment)

RED — failing regression committed alone

Commit: a7609584

Added non_object_action_params_do_not_replace_durable_fields. Before the fix, the action reached Running, but durable fields changed from:

Object {"durable": "keep"}

to:

Array ["transient"]

The test failed on that exact durability mismatch.

GREEN — root-cause fix

Commit: bb927a21

The shared actor ingress now projects action parameters into durable entity fields only when the parameters are a non-empty JSON object, matching the canonical server behavior. Scalar, array, and null parameters remain available to JIT effect evaluation but cannot replace the durable entity record.

Focused verification:

temper-actor-runtime: 13 passed; 0 failed
temper-runtime reaction tests: 8 passed; 0 failed
temper-cli: 13 relevant tests passed; 0 failed

Exact-head review gates:

cargo fmt --all -- --check                                  PASS
git diff --check origin/main...HEAD                         PASS
cargo clippy --workspace --all-targets -- -D warnings      PASS
readability ratchet blocking metrics                       PASS
RUST_TEST_THREADS=1 ... cargo test --workspace             PASS
git push pre-push pipeline (fmt/clippy/readability/tests)   ALL GATES PASSED
mandatory code-quality reviewer                            PASS (0 findings)
mandatory DST reviewer                                     PASS (0 findings; architecture CLEAN)

The PR head is now bb927a21. A fresh dedicated GPT-5.6 review of the complete open GitHub diff is being requested before Greptile is re-requested.

@nerdsane

Copy link
Copy Markdown
Owner Author

Dedicated GPT-5.6 re-review of the complete open GitHub PR diff at exact head bb927a21a3ba9f1eaf8c7c998a0cc942a25f5e0f.

The prior durable-field replacement blocker is fixed at its root. RED commit a7609584 isolates the failure: an array action payload completes the transition but replaces {"durable":"keep"} with ["transient"]. GREEN commit bb927a21 projects only non-empty JSON objects into SpecActorState.fields and continues passing the untouched local action_params to temper_jit::table::apply_effects. The focused regression passes, the reaction suite passes 8/8, and git diff --check origin/main...bb927a21 is clean. The earlier timer, resolver, cascade, fan-out, typed spawn-message, derived-spawn-budget, production mapping, and promotion-lock findings also remain resolved.

One non-security parameter-semantics blocker remains:

  1. [P2] A reaction whose declared static params are JSON null still reaches the target as {}, not null. crates/temper-actor-runtime/src/spec_actor/commands.rs:178-187 returns None whenever rule.then.params.is_null() and params_from is empty. route_rules consequently uses SpecMessage::routed, whose payload bytes are empty. At ingress, crates/temper-actor-runtime/src/spec_actor.rs:304-308 converts those empty bytes to json!({}) before effect evaluation. This differs from the canonical server path, where build_effective_params returns the declared ReactionTarget.params unchanged when params_from is empty, including Value::Null. It also falls short of the requested GREEN contract that scalar, array, and null reaction params remain available to target/effect evaluation while none are projected into durable fields. The RED test covers only an array delivered directly to handle, so it cannot detect this reaction-routing loss; the existing empty_parameter_reaction_does_not_copy_source_fields test currently asserts the lossy empty-byte representation. Preserve null in the routed payload (it will still project no durable fields under the new ingress logic) and add a routed-reaction regression that decodes null at the target while the target's pre-existing fields survive.

Verdict: FAIL

Copy link
Copy Markdown
Collaborator

Dedicated reviewer FAIL follow-up — routed JSON null RED/GREEN

The exact-head GPT-5.6 review at bb927a21 correctly returned Verdict: FAIL because the actor reaction adapter converted declared/default JSON null parameters into empty protobuf bytes. The receiving actor then substituted {}, so routed effect evaluation did not receive the canonical value used by the server path.

Reviewer verdict: #372 (comment)

RED — failing routed behavioral regression committed alone

Commit: 96f41e25

null_parameter_reaction_preserves_null_without_copying_source_fields exercises an actual reaction route and requires the target message to decode as JSON null. Before the fix it failed deterministically:

routed null params must remain JSON: Error("EOF while parsing a value", line: 1, column: 0)

That proves the route emitted empty bytes instead of the declared JSON value. The same assertion also proves source fields were not copied into an object.

GREEN — remove the lossy sentinel path

Commit: 1af07b6f

Removed the private SpecMessage::routed empty-byte path and the Option<Value> sentinel in reaction_params. Every reaction now serializes its canonical serde_json::Value uniformly through SpecMessage::with_params:

  • absent/default TOML params remain runtime Value::Null and route as JSON null;
  • explicit null, scalar, array, empty object, and non-empty object shapes are preserved;
  • params_from on null still materializes the same object used by the canonical server path;
  • the receiving actor continues to expose the decoded value to JIT effects while only non-empty objects may update durable fields.

The fix removes 16 net lines and leaves cascade depth and derived budgets unchanged.

Exact-head verification

routed-null RED before fix                                  FAIL (expected EOF)
routed-null regression after fix                            PASS
temper-actor-runtime --lib                                  13 passed; 0 failed
temper-runtime reaction tests                               8 passed; 0 failed
temper-agents serialized Postgres chain                     4 passed; 0 failed
temper-cli actor adapter tests                              13 passed; 0 failed
cargo fmt --all -- --check                                  PASS
git diff --check origin/main...HEAD                         PASS
bash scripts/readability-ratchet.sh check                   PASS (all blocking metrics)
cargo clippy --workspace --all-targets -- -D warnings      PASS
RUST_TEST_THREADS=1 ... cargo test --workspace             PASS
mandatory code-quality reviewer                            PASS (0 findings)
mandatory DST reviewer                                     PASS (0 findings; architecture CLEAN)
git push pre-push pipeline                                  ALL GATES PASSED

The first pre-push attempt encountered a parallel SQLite API-misuse failure in an unrelated server proof after 646/647 server tests passed. That proof passed in the serialized workspace run, passed 1/1 in isolation, and passed inside the subsequent complete unmodified pre-push run (647/647). No source or test weakening was applied.

The PR head is now 1af07b6f. A fresh dedicated GPT-5.6 review of the complete open GitHub diff is being requested before Greptile.

Copy link
Copy Markdown
Collaborator

Dedicated GPT-5.6 re-review of the complete open GitHub PR diff at exact head 1af07b6f757c8bfbc4b6126b01b7ec5b3ccb5eb3.

The routed-null blocker is resolved. RED commit 96f41e25 changes the actual reaction-routing regression to require a valid JSON null payload; on the preceding implementation it necessarily fails because reaction_params returns None, SpecMessage::routed emits empty bytes, and serde_json::from_slice cannot decode them. GREEN commit 1af07b6f removes that lossy branch: reaction_params returns a serde_json::Value for every canonical shape, SpecMessage::with_params serializes it, and route_rules then restores cascade_depth + 1 before buffering the tell.

The complete parameter path is now coherent with the canonical server:

  • null, scalar, array, and object values remain intact in action_params and are passed unchanged to temper_jit::table::apply_effects;
  • only non-empty objects are projected into SpecActorState.fields, so non-object inputs cannot discard durable fields;
  • empty/null reactions do not copy undeclared source state;
  • explicit params_from mappings remain deterministic and partial on missing source fields, with Id handled consistently;
  • command admission and cascade behavior are unchanged by the fix. Handler-derived tell/spawn budgets still cover maximum fan-out, and the protobuf-persisted depth is incremented after message construction and remains bounded by MAX_REACTION_DEPTH across restart.

Focused reviewer validation at this exact head passed: actor spec/reaction tests 13/13, runtime reaction-registry tests 8/8, production Temper Agents chain tests 4/4, and git diff --check origin/main...1af07b6f. I also rechecked the earlier scheduled-state, resolver commit, atomic spawn/tell flush, timer promotion ordering, schema initialization, typed message construction, server/JIT effect sharing, and production mapping changes; no actionable non-security correctness or durability finding remains in the open PR diff.

Verdict: PASS

Copy link
Copy Markdown
Collaborator

@greptile review

1 similar comment
@nerdsane

Copy link
Copy Markdown
Owner Author

@greptile review

Comment thread crates/temper-actor-runtime/src/pg.rs Outdated
@nerdsane

Copy link
Copy Markdown
Owner Author

Greptile finding — duplicate spawn initialization RED/GREEN

The identity-correct Greptile review found one additional non-security durability defect: CREATE_ACTOR is idempotent, but the durable flush path enqueued a spawn initial action even when the actor insert lost an ON CONFLICT DO NOTHING race. A repeated spawn could therefore leave one durable child with two initial messages.

Greptile thread: #372 (comment)

RED — failing PostgreSQL behavioral regression committed alone

Commit: 3c0a481c

duplicate_durable_spawn_enqueues_initial_action_once drives two durable parents that spawn the same child ID. Before the fix, PostgreSQL persisted one child actor but two initial messages:

$ TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' cargo test -p temper-actor-runtime --test effect_durability duplicate_durable_spawn_enqueues_initial_action_once -- --exact --nocapture
running 1 test
assertion `left == right` failed: an existing durable child must not receive the initial action again
  left: 2
 right: 1
test duplicate_durable_spawn_enqueues_initial_action_once ... FAILED

GREEN — atomic one-time initialization

Commit: 23ecb505

The parent activation transaction now retains the affected-row count from CREATE_ACTOR. Only the transaction that actually inserts the child (created == 1) may enqueue its initial message. Conflicting or repeated inserts return zero and do not replay initialization; child creation, message insertion, parent state, cursor, and other commands remain in the same transaction.

Focused regression after the fix:

$ TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_final' cargo test -p temper-actor-runtime --test effect_durability duplicate_durable_spawn_enqueues_initial_action_once -- --exact --nocapture
running 1 test
test duplicate_durable_spawn_enqueues_initial_action_once ... ok
test result: ok. 1 passed; 0 failed

Fresh-database live PostgreSQL E2E:

$ createdb -h 127.0.0.1 -p 55432 -U postgres arn179_greptile_fix
$ RUST_TEST_THREADS=1 TEMPER_TEST_DATABASE_URL='host=127.0.0.1 port=55432 user=postgres dbname=arn179_greptile_fix' cargo test -p temper-actor-runtime --test effect_durability -- --nocapture
running 5 tests
test command_flush_failure_rolls_back_parent_state_cursor_and_commands ... ok
test concurrent_schema_initializers_wait_for_the_global_ddl_lock ... ok
test concurrent_timer_promoters_preserve_global_due_order ... ok
test duplicate_durable_spawn_enqueues_initial_action_once ... ok
test postgres_effects_survive_restart_and_commit_runtime_commands ... ok
test result: ok. 5 passed; 0 failed

Exact-head gates:

cargo fmt --all -- --check                              PASS
git diff --check origin/main...HEAD                     PASS
cargo clippy -p temper-actor-runtime --all-targets -- -D warnings  PASS
bash scripts/readability-ratchet.sh check               PASS
RUST_TEST_THREADS=1 ... cargo test --workspace          PASS (exit 0)
mandatory code-quality review                           PASS (0 findings)
mandatory DST review                                    PASS (0 findings; architecture CLEAN)
git push pre-push pipeline                              ALL GATES PASSED

The PR head is now 23ecb505. A dedicated GPT-5.6 reviewer is re-reviewing the complete open GitHub diff before Greptile is requested again.

@nerdsane

Copy link
Copy Markdown
Owner Author

Dedicated GPT-5.6 review of the complete open GitHub PR diff at exact head 23ecb50507e4c88699de43bd54b901ea953e999c.

No actionable non-security correctness or durability finding remains.

The duplicate-spawn regression is correctly isolated and fixed:

  • RED 3c0a481c drives two durable parents that request the same child identity and proves the previous flush path persisted one child row but two initialization messages.
  • GREEN 23ecb505 retains the affected-row count from CREATE_ACTOR ... ON CONFLICT DO NOTHING and enqueues the optional initial message only when created == 1. That is the atomic ownership signal: sequential duplicates and concurrent insert races both leave initialization to the transaction that actually inserted the child. Child creation, initialization, parent state/cursor persistence, and all other buffered commands remain in one transaction, so a later failure rolls the complete activation back.

I re-audited the full GitHub-hosted origin/main...23ecb505 patch, including the exhaustive shared JIT effect executor, server adapter parity, transactional tells/schedules/spawns, timer promotion ordering, schema initialization, reaction fan-out and target resolution, canonical JSON parameter shapes, durable cascade depth, derived command budgets, typed spawn messages, CLI tenant-definition checks, and Temper Agents mappings. The previous scheduled-state, resolver, fan-out, parameter, and routed-null blockers remain resolved, and the new spawn gate introduces no budget or ordering change.

GitHub checks visible during review were green for compile/lint, integrity and DST patterns, verification contract, spec verification, instrumentation hygiene, and platform-random DST; the remaining long-running suites were still pending and do not change this code-review verdict.

Verdict: PASS

@nerdsane

Copy link
Copy Markdown
Owner Author

@greptile review

@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

@nerdsane

Copy link
Copy Markdown
Owner Author

ARENA SHIPPABLE · Codex (GPT-5.6) · 2026-07-13 12:16 PDT

Final audit:

  • canonical exhaustive effect execution is shared across JIT/server/PostgreSQL actor paths;
  • durable state, tells, timers, spawns, reaction parameters, cascade budgets, and rollback/restart behavior are covered;
  • duplicate/repeated durable spawns create one child and enqueue the initial action exactly once;
  • mandatory code-quality review: PASS, 0 findings;
  • mandatory DST review: PASS, 0 findings, architecture CLEAN;
  • local cargo fmt, diff check, strict Clippy, readability ratchet, full workspace tests, and the four-gate pre-push pipeline passed;
  • final thread-aware snapshot: 5 review threads, 0 unresolved; every actionable Greptile finding has an explicit response;
  • PR remains open and unmerged for arena adjudication.

@rita-aga

Copy link
Copy Markdown
Collaborator

Closing this ARN-179 competition entry. Adjudicated in favor of Fable #370 (completeness/scope). This branch is preserved for grafting its best parts into the winner; full scoring is on the ARN-165 arena board. (Rita-directed close, 2026-08-11.)

@rita-aga rita-aga closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants