[sfmskywalker]

Self-review of #233 (Aster.Elsa.Agents, program #58). The review found two defects, both reproduced against the real store.

Changes

• A reference to a deleted agent blocked every later edit of the agent that used it (medium).
  • Before:
    • On read, a referenced agent's ID becomes its name even when that agent is soft-deleted.
    • On save, names resolved only among agents that weren't deleted.
    • So the name the store handed out could not be turned back into an ID, and strict reference validation rejected the save (reference-target-missing).
  • Scenario: Writer references Researcher. Researcher is deleted. Editing Writer's prompt in Elsa then fails.
  • Fix: names now also resolve through the agent's current references, using the same name lookup as the read path. That lookup was factored into ReadNamesAsync. A reference to a deleted agent now saves as it reads. The validator only checks that the target exists, so a soft-deleted target is valid.
• Updating a deleted or archived agent made the edit vanish (low).
  • Before: UpdateAsync saved and activated a new version, but the marker kept it invisible.
  • Scenario: user A deletes an agent while user B still has it open. B's save "succeeds" and disappears.
  • Fix: it now throws InvalidOperationException, as Elsa's EF Core store throws on a deleted row. Bringing an agent back stays an explicit IResourceLifecycleRestoreService call.
  • The importer is unaffected: it already skips any ID that exists in any lifecycle state.
• The README is updated (UpdateAsync row, reference resolution order).

Tests

• New tests, each run against every provider (in-memory, SQLite, Postgres, SQL Server):
  • Updating_a_deleted_agent_throws_and_keeps_it_deleted
  • An_agent_that_references_a_deleted_agent_saves_as_it_reads
• Both fail on main and pass with the fix.
• Aster.Elsa.Agents.Tests: 114 passed, 1 skipped (benchmark) on each of net8.0, net9.0 and net10.0.

Rest of the self-review

• Core and SQL Server (Portability: match definitions by content, not generated IDs #226, Lifecycle hooks: tell after hooks whether the operation succeeded #227, Add the SQL Server JSON provider on the shared relational core #228, Identifiers: reject leading and trailing whitespace in core #231, core part of Elsa: triggers on resource lifecycle events; archive and restore hook points #232): no finding survived verification.
• Studio (Add Aster.Elsa.Studio: Elsa Studio module with schema-driven resource editors (#117) #234) and Elsa triggers (Add Aster.Elsa: Elsa workflow activities over Aster resources #229, Elsa: triggers on resource lifecycle events; archive and restore hook points #232): fixes follow in separate PRs.

---

Generated by Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Prevented updates to archived or deleted agents, preserving their existing state until restored.
  • Preserved references to deleted agents when saving and reading agent configurations.
  • Improved agent-name resolution to retain valid references across lifecycle states.

• Documentation

  • Updated agent store guidance to reflect restrictions on updating archived or deleted agents.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: ee0321b4-6130-466c-9cc2-daf070df3656

📥 Commits

Reviewing files that changed from the base of the PR and between 337eaed and 9826c41.

📒 Files selected for processing: 3

• src/integrations/Aster.Elsa.Agents/AsterAgentStore.cs
• src/integrations/Aster.Elsa.Agents/README.md
• test/Aster.Elsa.Agents.Tests/AsterAgentStoreTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The agent store now rejects updates to archived or deleted agents. It also preserves references to agents that have since been deleted by resolving names against the current agent’s stored reference IDs.

Changes: Agent store behavior

Layer / File(s)	Summary
Reject updates to archived or deleted agents src/integrations/Aster.Elsa.Agents/AsterAgentStore.cs, src/integrations/Aster.Elsa.Agents/README.md, test/Aster.Elsa.Agents.Tests/AsterAgentStoreTests.cs	SaveAsync rejects updates when an agent has a non-None lifecycle marker. The README describes the behavior. Provider-parameterized tests verify that a deleted agent remains unreadable and retains one version after a rejected update.
Preserve references to deleted agents src/integrations/Aster.Elsa.Agents/AsterAgentStore.cs, src/integrations/Aster.Elsa.Agents/README.md, test/Aster.Elsa.Agents.Tests/AsterAgentStoreTests.cs	Reference-name lookup checks active versions, then latest versions. Unresolved names are retried against the current agent’s stored reference IDs. Documentation and tests cover retaining the deleted agent’s resource ID during an update.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Possibly related issues

• Issue 58 describes missing resource references as a product gap. This change preserves references to deleted Elsa agents.
• Issue 114 includes persistence alignment for the Elsa module. This change updates persistence behavior for deleted, archived, and referenced agents, but does not address packaging or dependency-injection design.

Merge Risk: ⚪ Minimal · up to 9826c

The changed update and reference-preservation paths show no actionable merge-blocking risk. The PR is mergeable after normal checks.

Security architecture risk: 🔵 Low · up to 9826c

The change blocks saves to deleted or archived agents and preserves existing references without establishing a new cross-tenant access path. Concurrent delete/save coordination remains uncertain.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The newly enabled save behavior is bounded to references already stored on the resource being edited within the current tenant scope. No new cross-service authority, credential access, or arbitrary deleted-resource lookup is established by this change.

Trust Boundaries and Controls

• observed — Write scope comes from CurrentTenant rather than the submitted entity's TenantId. Marker checks, resource writes, and reference lookups receive that scope. This establishes consistent scope propagation, not independent verification of provider isolation or caller authorization.

Resilience and Maintainability Implications

• inferred — The new guard does not establish atomic lifecycle enforcement: deletion can occur after its marker read, and version-conflict retries do not repeat that check. Separate marker and version persistence already existed at the base, so this residual ownership race is not attributed to the PR. Provider-level serialization remains unverified.

Hardening Proposals

• proposed — If lifecycle markers must prohibit every concurrent write, make marker validation and version persistence a shared conditional operation coordinated with delete/archive, rather than treating the preflight guard as that guarantee.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes both main changes: preserving references to deleted agents and rejecting saves to deleted agents.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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