Add versioned agent profiles, skills and enterprise agent administration - #81
Closed
jusso-dev wants to merge 1 commit into
Closed
Add versioned agent profiles, skills and enterprise agent administration#81jusso-dev wants to merge 1 commit into
jusso-dev wants to merge 1 commit into
Conversation
…rollback Adds a first-class agent_profile_versions table (draft -> approved -> active -> retired) plus agent_policies (model/memory/tool/escalation) and agent_profile_evaluations, alongside the existing skill lifecycle. Every run now records the exact agentProfileVersionId it used so historical runs stay explainable after later profile changes. Self-approval is rejected both in the service layer and by a DB CHECK constraint (approved_by_actor_id <> created_by_actor_id). Evaluation must pass before approval; activation requires the approved state; rollback restores the immediate predecessor. Fixes a kill-switch TOCTOU race in the harness invoke path with an in-transaction re-check. Seeds Alfie, Jessie, and Parker with distinct active governed profiles in bootstrap and demo seed data. Adds an admin "Versions" panel (evaluate/approve/reject/activate/roll back/retire) and an everyday profile summary (role, communication style, example prompts, availability, recent room work) gated server-side by agents.manage — non-admins never receive draft content or operating instructions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds governed, versioned agent profiles with database persistence, evaluation and approval workflows, runtime profile attribution, seeded configurations, API handlers, and administrator UI controls for lifecycle management. ChangesAgent profile governance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Administrator
participant ProfileUI
participant ProfileRoute
participant AgentProfileDomain
participant Database
Administrator->>ProfileUI: propose or manage profile version
ProfileUI->>ProfileRoute: POST profile mutation
ProfileRoute->>AgentProfileDomain: authorize and dispatch mutation
AgentProfileDomain->>Database: persist lifecycle transition and audit data
Database-->>AgentProfileDomain: updated profile state
AgentProfileDomain-->>ProfileRoute: mutation result
ProfileRoute-->>ProfileUI: JSON response with traceId
ProfileUI->>ProfileRoute: reload profile state
ProfileRoute-->>ProfileUI: governed versions and active profile
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agent-harness/src/index.ts (1)
221-262: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftAccept runs from one locked, current agent-definition snapshot.
The harness locks kill-switch/status but records the pre-transaction profile ID; the gateway does not recheck exposure inside its transaction. A concurrent activation can store a stale/null profile, and the gateway can queue a run after the kill switch is enabled.
packages/agent-harness/src/index.ts#L221-L262: selectactiveProfileVersionIdwith the locked row, reject a null value, and insert that locked value.apps/agent-gateway/src/index.ts#L107-L107: inside the transaction, reselect the definition withFOR UPDATE, scoped by organisation; recheck status/kill switch, require an active profile ID, and use that row for the insert.As per coding guidelines, “Scope every domain query by organisation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-harness/src/index.ts` around lines 221 - 262, Update the harness transaction around the agent-definition lock to select activeProfileVersionId, reject missing profiles, and insert that locked value; in apps/agent-gateway/src/index.ts at line 107, reselect the definition inside its transaction with FOR UPDATE scoped by organisation, recheck active status and killSwitch, require an active profile ID, and use the locked row for insertion.Source: Coding guidelines
🧹 Nitpick comments (5)
packages/database/migrations/0021_last_loki.sql (1)
87-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd the existing-run foreign key without inline validation.
This FK can scan and block writes to
agent_runsduring deployment. Add itNOT VALID, then validate it in a later migration.Proposed migration change
-ALTER TABLE "agent_runs" ADD CONSTRAINT "agent_runs_agent_profile_version_id_agent_profile_versions_id_fk" FOREIGN KEY ("agent_profile_version_id") REFERENCES "public"."agent_profile_versions"("id") ON DELETE no action ON UPDATE no action; +ALTER TABLE "agent_runs" ADD CONSTRAINT "agent_runs_agent_profile_version_id_agent_profile_versions_id_fk" FOREIGN KEY ("agent_profile_version_id") REFERENCES "public"."agent_profile_versions"("id") ON DELETE no action ON UPDATE no action NOT VALID; + +-- Subsequent migration: +ALTER TABLE "agent_runs" VALIDATE CONSTRAINT "agent_runs_agent_profile_version_id_agent_profile_versions_id_fk";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/0021_last_loki.sql` at line 87, Update the foreign-key constraint statement on agent_runs to add it as NOT VALID, preserving the existing referenced table, columns, and delete/update actions. Do not validate it in this migration; leave validation for a later migration.Source: Linters/SAST tools
apps/web/lib/agent-profile-domain.ts (1)
725-734: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
reasonredaction is inconsistent across lifecycle actions.Only the rollback path passes
reasonthroughredactObservationText;approveProfile(Line 567),rejectProfile(Line 682),activateProfile(Line 623), andretireProfile(Line 774) persist the operator-supplied string verbatim into audit metadata. Pick one policy — either redact all of them or none — so audit records are uniform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/agent-profile-domain.ts` around lines 725 - 734, Make reason handling consistent across the lifecycle actions: update approveProfile, activateProfile, rejectProfile, and retireProfile to apply the same redactObservationText policy used by the rollback audit event before persisting reason in metadata. Preserve the existing audit actions and metadata structure.tests/agents-admin.spec.ts (1)
43-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEach run leaves a new draft version and pending approval on the seeded Alfie. Nothing removes the proposed version afterwards, so repeated suite runs accumulate drafts and pending
agent.profile.approverows against the seeded agent. Retire the version in afinally/afterEach, or use the API to reject it once the assertions pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agents-admin.spec.ts` around lines 43 - 68, Clean up the proposed profile created by the governance test after each run. Use the captured versionId to reject or retire the draft through the existing API in a finally/afterEach path, ensuring cleanup runs even when assertions fail and preventing pending approval rows from accumulating on the seeded Alfie agent.apps/web/components/agents-view.tsx (1)
1087-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Forbidden" is inferred by sniffing for a
versionskey.If the API ever returns a non-2xx for insufficient capability,
loadthrows first (Line 1087) and the operator gets a red error banner rather than the access-denied panel. Keying off the response status (or an explicit flag in the payload) is more durable than shape detection.♻️ Handle the status explicitly
+ if (response.status === 403) { + setProfile("forbidden"); + return; + } if (!response.ok || !payload.data) { throw new Error( payload.detail ?? "Profile versions could not be loaded", ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/agents-view.tsx` around lines 1087 - 1096, Update the profile-loading logic in the load flow to detect insufficient capability from the API response status or an explicit authorization flag before the generic !response.ok error is thrown. Set the profile to "forbidden" and return for that case, while preserving the existing error handling for other failures and normal AdminProfileState processing.apps/web/lib/agent-profile-domain.integration.test.ts (1)
17-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo teardown for the rows this suite creates.
afterAllonly closes the connection. The synthetic actor, profile versions, policies, and the injectedagentRunsrow persist and accumulate on every run against a shared database. Consider deleting the created ids (runs → evaluations/approvals → versions → policies → actor) inafterAllbeforecloseDatabase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/agent-profile-domain.integration.test.ts` around lines 17 - 40, The suite’s afterAll teardown only closes the database and leaves its seeded test data behind. Track the created actor and all profile-related rows, including injected agentRuns, evaluations, approvals, versions, and policies, then delete them in dependency order (runs, evaluations/approvals, versions, policies, actor) before invoking closeDatabase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/components/agents-view.tsx`:
- Line 1141: Update the versions rendering flow near the profile version empty
state to distinguish an unresolved profile from a loaded profile with no
versions. Use the existing profile loading/null state to render the loading
branch before deriving or displaying the “No profile versions proposed yet”
message, while preserving the empty state once profile has loaded and versions
is genuinely empty.
- Around line 1228-1243: Update the evaluation action in the version controls
around the draft-state button so drafts with an existing failed evaluation can
invoke evaluate_profile again. Keep the button hidden for drafts without the
required evaluation state only as appropriate, and label it “Re-evaluate” when
version.evaluation exists but did not pass while preserving “Evaluate” for
unevaluated drafts.
In `@apps/web/lib/agent-profile-domain.ts`:
- Around line 308-321: Scope all affected domain queries by organisation: in the
version lookups around the latest and current queries, add the organisationId
predicate using context.organisationId; also add the same predicate to the
agentPolicies version lookup. Apply these changes at
apps/web/lib/agent-profile-domain.ts lines 308-321 and 787-795, preserving the
existing agent, state, and max(version) conditions.
- Around line 144-162: Replace organisation-wide approval scans with indexed
lookups in both sites: in apps/web/lib/agent-profile-domain.ts lines 144-162,
constrain evaluations with inArray(profileVersionId, versionIds) and approvals
with inArray(idempotencyKey, versionIds.map(...)), then remove the related
in-memory find; in lines 405-422, query approvals using the organisation scope
plus eq(idempotencyKey, `agent.profile.approve:${versionId}`) and remove its
find. Update the affected agent-profile lookup flow without changing its
returned behavior.
- Around line 499-542: Guard lifecycle state transitions inside their
transactions: update approveProfile and the analogous activateProfile,
rejectProfile, rollbackProfile, and retireProfile operations to include the
expected current state in each UPDATE predicate, rather than relying on the
pre-transaction check. Detect when no row is returned or affected and fail the
operation, preventing concurrent calls from applying transitions to an already
changed version.
In `@packages/database/src/bootstrap.ts`:
- Around line 314-324: Make policy and profile reseeding append-only: in
packages/database/src/bootstrap.ts lines 314-324 and 441-457, and
packages/database/src/seed.ts lines 397-407 and 518-534, stop updating existing
version-1 records through the conflict handlers. Preserve existing immutable
versions and create governed successor versions when policy documents or profile
content changes, including the corresponding version and audit/hash
relationships.
- Around line 398-411: Use the canonical profile hash payload for both seed
paths: update computeProfileContentHash usage in
packages/database/src/bootstrap.ts at lines 398-411 and
packages/database/src/seed.ts at lines 475-488 to include avatarAssetId: null,
preferably by reusing the shared helper or payload used by
prepareProfileProposal. Keep all other profile fields unchanged so equivalent
profiles produce identical hashes.
- Around line 413-440: Update the seeded profile lifecycle so both
packages/database/src/bootstrap.ts lines 413-440 and
packages/database/src/seed.ts lines 490-517 persist a passing
agent_profile_evaluations record before approving or activating the profile,
then retain the active state only after that evaluation gate is satisfied;
alternatively, implement an explicit audited bootstrap exemption consistently in
both sites.
---
Outside diff comments:
In `@packages/agent-harness/src/index.ts`:
- Around line 221-262: Update the harness transaction around the
agent-definition lock to select activeProfileVersionId, reject missing profiles,
and insert that locked value; in apps/agent-gateway/src/index.ts at line 107,
reselect the definition inside its transaction with FOR UPDATE scoped by
organisation, recheck active status and killSwitch, require an active profile
ID, and use the locked row for insertion.
---
Nitpick comments:
In `@apps/web/components/agents-view.tsx`:
- Around line 1087-1096: Update the profile-loading logic in the load flow to
detect insufficient capability from the API response status or an explicit
authorization flag before the generic !response.ok error is thrown. Set the
profile to "forbidden" and return for that case, while preserving the existing
error handling for other failures and normal AdminProfileState processing.
In `@apps/web/lib/agent-profile-domain.integration.test.ts`:
- Around line 17-40: The suite’s afterAll teardown only closes the database and
leaves its seeded test data behind. Track the created actor and all
profile-related rows, including injected agentRuns, evaluations, approvals,
versions, and policies, then delete them in dependency order (runs,
evaluations/approvals, versions, policies, actor) before invoking closeDatabase.
In `@apps/web/lib/agent-profile-domain.ts`:
- Around line 725-734: Make reason handling consistent across the lifecycle
actions: update approveProfile, activateProfile, rejectProfile, and
retireProfile to apply the same redactObservationText policy used by the
rollback audit event before persisting reason in metadata. Preserve the existing
audit actions and metadata structure.
In `@packages/database/migrations/0021_last_loki.sql`:
- Line 87: Update the foreign-key constraint statement on agent_runs to add it
as NOT VALID, preserving the existing referenced table, columns, and
delete/update actions. Do not validate it in this migration; leave validation
for a later migration.
In `@tests/agents-admin.spec.ts`:
- Around line 43-68: Clean up the proposed profile created by the governance
test after each run. Use the captured versionId to reject or retire the draft
through the existing API in a finally/afterEach path, ensuring cleanup runs even
when assertions fail and preventing pending approval rows from accumulating on
the seeded Alfie agent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79811e28-195e-42fd-9c6d-ccfecaea2bda
📒 Files selected for processing (16)
apps/agent-gateway/src/index.tsapps/web/app/api/v1/agents/[id]/profile/route.tsapps/web/components/agents-view.tsxapps/web/lib/agent-profile-domain.integration.test.tsapps/web/lib/agent-profile-domain.tspackages/agent-harness/src/index.tspackages/agents/src/index.tspackages/agents/src/profile-governance.test.tspackages/database/migrations/0021_last_loki.sqlpackages/database/migrations/meta/0021_snapshot.jsonpackages/database/migrations/meta/_journal.jsonpackages/database/src/bootstrap.tspackages/database/src/schema.tspackages/database/src/seed-data.tspackages/database/src/seed.tstests/agents-admin.spec.ts
| ); | ||
| } | ||
|
|
||
| const versions = profile?.versions ?? []; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Loading state is rendered as "no versions proposed yet".
profile is null until the first fetch resolves, so versions is [] and the empty state shows immediately — an administrator briefly sees a false "No profile versions proposed yet." Distinguish "not loaded" from "loaded and empty".
♻️ Separate the loading branch
- const versions = profile?.versions ?? [];
+ const loaded = profile !== null;
+ const versions = profile?.versions ?? [];- {versions.length === 0 ? (
+ {!loaded && !error ? (
+ <div className="p-8 text-center text-xs text-muted-foreground">
+ Loading…
+ </div>
+ ) : versions.length === 0 ? (Also applies to: 1176-1182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/components/agents-view.tsx` at line 1141, Update the versions
rendering flow near the profile version empty state to distinguish an unresolved
profile from a loaded profile with no versions. Use the existing profile
loading/null state to render the loading branch before deriving or displaying
the “No profile versions proposed yet” message, while preserving the empty state
once profile has loaded and versions is genuinely empty.
Comment on lines
+1228
to
+1243
| {version.state === "draft" && !version.evaluation && ( | ||
| <Button | ||
| size="sm" | ||
| variant="outline" | ||
| disabled={pending === version.id} | ||
| onClick={() => | ||
| void mutate( | ||
| { action: "evaluate_profile", versionId: version.id }, | ||
| version.id, | ||
| ) | ||
| } | ||
| > | ||
| <ShieldCheck /> | ||
| Evaluate | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No path to re-evaluate a draft after a failed evaluation. The Evaluate button is gated on !version.evaluation, and Approve is disabled when !evaluation.passed — so a draft with a failing score can only be rejected or retired, even though the domain layer allows repeated evaluate_profile calls. Consider showing "Re-evaluate" when an evaluation exists but did not pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/components/agents-view.tsx` around lines 1228 - 1243, Update the
evaluation action in the version controls around the draft-state button so
drafts with an existing failed evaluation can invoke evaluate_profile again.
Keep the button hidden for drafts without the required evaluation state only as
appropriate, and label it “Re-evaluate” when version.evaluation exists but did
not pass while preserving “Evaluate” for unevaluated drafts.
Comment on lines
+144
to
+162
| const evaluations = versionRows.length | ||
| ? await db | ||
| .select() | ||
| .from(schema.agentProfileEvaluations) | ||
| .where( | ||
| eq(schema.agentProfileEvaluations.organisationId, subject.organisationId), | ||
| ) | ||
| .orderBy(desc(schema.agentProfileEvaluations.createdAt)) | ||
| : []; | ||
| const approvals = await db | ||
| .select() | ||
| .from(schema.approvals) | ||
| .where( | ||
| and( | ||
| eq(schema.approvals.organisationId, subject.organisationId), | ||
| eq(schema.approvals.actionType, "agent.profile.approve"), | ||
| ), | ||
| ) | ||
| .orderBy(desc(schema.approvals.requestedAt)); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Approvals are located by scanning the whole organisation and matching in JavaScript. Both sites load every agent.profile.approve row for the organisation and then filter on target.profileVersionId. The write path already stores a deterministic idempotencyKey of agent.profile.approve:${versionId}, so both can be indexed lookups.
apps/web/lib/agent-profile-domain.ts#L144-L162: constrain the evaluations query byinArray(profileVersionId, versionIds)and the approvals query byinArray(idempotencyKey, versionIds.map(...)), dropping the in-memoryfindon Lines 170-179.apps/web/lib/agent-profile-domain.ts#L405-L422: replace the org-wide select plusfindwith a singleeq(schema.approvals.idempotencyKey, \agent.profile.approve:${versionId}`)` lookup scoped by organisation.
📍 Affects 1 file
apps/web/lib/agent-profile-domain.ts#L144-L162(this comment)apps/web/lib/agent-profile-domain.ts#L405-L422
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/agent-profile-domain.ts` around lines 144 - 162, Replace
organisation-wide approval scans with indexed lookups in both sites: in
apps/web/lib/agent-profile-domain.ts lines 144-162, constrain evaluations with
inArray(profileVersionId, versionIds) and approvals with inArray(idempotencyKey,
versionIds.map(...)), then remove the related in-memory find; in lines 405-422,
query approvals using the organisation scope plus eq(idempotencyKey,
`agent.profile.approve:${versionId}`) and remove its find. Update the affected
agent-profile lookup flow without changing its returned behavior.
Comment on lines
+308
to
+321
| const [latest] = await tx | ||
| .select({ version: max(schema.agentProfileVersions.version) }) | ||
| .from(schema.agentProfileVersions) | ||
| .where(eq(schema.agentProfileVersions.agentId, context.agentId)); | ||
| const [current] = await tx | ||
| .select({ id: schema.agentProfileVersions.id }) | ||
| .from(schema.agentProfileVersions) | ||
| .where( | ||
| and( | ||
| eq(schema.agentProfileVersions.agentId, context.agentId), | ||
| eq(schema.agentProfileVersions.state, "active"), | ||
| ), | ||
| ) | ||
| .limit(1); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Two max(version) lookups omit the organisation predicate. Both rely on an earlier requireAgent call for tenant safety rather than scoping the query itself.
apps/web/lib/agent-profile-domain.ts#L308-L321: addeq(schema.agentProfileVersions.organisationId, context.organisationId)to both themax(version)and the active-version lookups.apps/web/lib/agent-profile-domain.ts#L787-L795: addeq(schema.agentPolicies.organisationId, context.organisationId)to the policymax(version)lookup.
As per coding guidelines, "Scope every domain query by organisation."
📍 Affects 1 file
apps/web/lib/agent-profile-domain.ts#L308-L321(this comment)apps/web/lib/agent-profile-domain.ts#L787-L795
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/agent-profile-domain.ts` around lines 308 - 321, Scope all
affected domain queries by organisation: in the version lookups around the
latest and current queries, add the organisationId predicate using
context.organisationId; also add the same predicate to the agentPolicies version
lookup. Apply these changes at apps/web/lib/agent-profile-domain.ts lines
308-321 and 787-795, preserving the existing agent, state, and max(version)
conditions.
Source: Coding guidelines
Comment on lines
+499
to
+542
| const record = await versionContext(context, versionId); | ||
| if (record.state !== "draft") { | ||
| throw new Error("Only a draft profile version can be approved"); | ||
| } | ||
| const [evaluation] = await database() | ||
| .select() | ||
| .from(schema.agentProfileEvaluations) | ||
| .where( | ||
| and( | ||
| eq(schema.agentProfileEvaluations.organisationId, context.organisationId), | ||
| eq(schema.agentProfileEvaluations.profileVersionId, versionId), | ||
| ), | ||
| ) | ||
| .orderBy(desc(schema.agentProfileEvaluations.createdAt)) | ||
| .limit(1); | ||
| const approval = await approvalForVersion(context, versionId); | ||
| if (!evaluation || !approval || approval.status !== "pending") { | ||
| throw new Error("Pending approval and completed evaluation are required"); | ||
| } | ||
| const decision = mayApproveProfile( | ||
| { | ||
| passed: evaluation.passed, | ||
| score: evaluation.score, | ||
| ...(evaluation.baselineScore !== null | ||
| ? { baselineScore: evaluation.baselineScore } | ||
| : {}), | ||
| regressions: strings(evaluation.regressions), | ||
| }, | ||
| record.createdByActorId, | ||
| context.actorId, | ||
| ); | ||
| if (!decision.allowed) throw new Error(decision.reasons.join("; ")); | ||
| const now = new Date(); | ||
| return database().transaction(async (tx) => { | ||
| const [approved] = await tx | ||
| .update(schema.agentProfileVersions) | ||
| .set({ | ||
| state: "approved", | ||
| approvedByActorId: context.actorId, | ||
| approvedAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .where(eq(schema.agentProfileVersions.id, versionId)) | ||
| .returning(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
State is checked outside the transaction, then updated without a state predicate.
record.state is read on Line 499, the transaction opens on Line 532, and the UPDATE matches on id alone. Two concurrent lifecycle calls (approve + reject, or two approves) both pass their pre-checks and both write, so a rejected/retired version can be flipped back to approved. Make the transition conditional on the expected state and fail when no row matched.
🔒️ Guard the transition inside the transaction
const [approved] = await tx
.update(schema.agentProfileVersions)
.set({
state: "approved",
approvedByActorId: context.actorId,
approvedAt: now,
updatedAt: now,
})
- .where(eq(schema.agentProfileVersions.id, versionId))
+ .where(
+ and(
+ eq(schema.agentProfileVersions.id, versionId),
+ eq(schema.agentProfileVersions.organisationId, context.organisationId),
+ eq(schema.agentProfileVersions.state, "draft"),
+ ),
+ )
.returning();
+ if (!approved) {
+ throw new Error("Profile version is no longer a draft");
+ }The same unguarded-update shape appears in activateProfile, rejectProfile, rollbackProfile, and retireProfile.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const record = await versionContext(context, versionId); | |
| if (record.state !== "draft") { | |
| throw new Error("Only a draft profile version can be approved"); | |
| } | |
| const [evaluation] = await database() | |
| .select() | |
| .from(schema.agentProfileEvaluations) | |
| .where( | |
| and( | |
| eq(schema.agentProfileEvaluations.organisationId, context.organisationId), | |
| eq(schema.agentProfileEvaluations.profileVersionId, versionId), | |
| ), | |
| ) | |
| .orderBy(desc(schema.agentProfileEvaluations.createdAt)) | |
| .limit(1); | |
| const approval = await approvalForVersion(context, versionId); | |
| if (!evaluation || !approval || approval.status !== "pending") { | |
| throw new Error("Pending approval and completed evaluation are required"); | |
| } | |
| const decision = mayApproveProfile( | |
| { | |
| passed: evaluation.passed, | |
| score: evaluation.score, | |
| ...(evaluation.baselineScore !== null | |
| ? { baselineScore: evaluation.baselineScore } | |
| : {}), | |
| regressions: strings(evaluation.regressions), | |
| }, | |
| record.createdByActorId, | |
| context.actorId, | |
| ); | |
| if (!decision.allowed) throw new Error(decision.reasons.join("; ")); | |
| const now = new Date(); | |
| return database().transaction(async (tx) => { | |
| const [approved] = await tx | |
| .update(schema.agentProfileVersions) | |
| .set({ | |
| state: "approved", | |
| approvedByActorId: context.actorId, | |
| approvedAt: now, | |
| updatedAt: now, | |
| }) | |
| .where(eq(schema.agentProfileVersions.id, versionId)) | |
| .returning(); | |
| const record = await versionContext(context, versionId); | |
| if (record.state !== "draft") { | |
| throw new Error("Only a draft profile version can be approved"); | |
| } | |
| const [evaluation] = await database() | |
| .select() | |
| .from(schema.agentProfileEvaluations) | |
| .where( | |
| and( | |
| eq(schema.agentProfileEvaluations.organisationId, context.organisationId), | |
| eq(schema.agentProfileEvaluations.profileVersionId, versionId), | |
| ), | |
| ) | |
| .orderBy(desc(schema.agentProfileEvaluations.createdAt)) | |
| .limit(1); | |
| const approval = await approvalForVersion(context, versionId); | |
| if (!evaluation || !approval || approval.status !== "pending") { | |
| throw new Error("Pending approval and completed evaluation are required"); | |
| } | |
| const decision = mayApproveProfile( | |
| { | |
| passed: evaluation.passed, | |
| score: evaluation.score, | |
| ...(evaluation.baselineScore !== null | |
| ? { baselineScore: evaluation.baselineScore } | |
| : {}), | |
| regressions: strings(evaluation.regressions), | |
| }, | |
| record.createdByActorId, | |
| context.actorId, | |
| ); | |
| if (!decision.allowed) throw new Error(decision.reasons.join("; ")); | |
| const now = new Date(); | |
| return database().transaction(async (tx) => { | |
| const [approved] = await tx | |
| .update(schema.agentProfileVersions) | |
| .set({ | |
| state: "approved", | |
| approvedByActorId: context.actorId, | |
| approvedAt: now, | |
| updatedAt: now, | |
| }) | |
| .where( | |
| and( | |
| eq(schema.agentProfileVersions.id, versionId), | |
| eq(schema.agentProfileVersions.organisationId, context.organisationId), | |
| eq(schema.agentProfileVersions.state, "draft"), | |
| ), | |
| ) | |
| .returning(); | |
| if (!approved) { | |
| throw new Error("Profile version is no longer a draft"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/agent-profile-domain.ts` around lines 499 - 542, Guard lifecycle
state transitions inside their transactions: update approveProfile and the
analogous activateProfile, rejectProfile, rollbackProfile, and retireProfile
operations to include the expected current state in each UPDATE predicate,
rather than relying on the pre-transaction check. Detect when no row is returned
or affected and fail the operation, preventing concurrent calls from applying
transitions to an already changed version.
Comment on lines
+314
to
+324
| .onConflictDoUpdate({ | ||
| target: [ | ||
| schema.agentPolicies.agentId, | ||
| schema.agentPolicies.kind, | ||
| schema.agentPolicies.version, | ||
| ], | ||
| set: { | ||
| document: sql`excluded.document`, | ||
| updatedAt: sql`now()`, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not overwrite immutable policy or profile versions during reseeding.
These conflict handlers mutate active version-1 policy documents and profile content in place, defeating version history and making prior hashes/audits describe changed content.
packages/database/src/bootstrap.ts#L314-L324: leave an existing policy version unchanged; create a new version for changed policy content.packages/database/src/bootstrap.ts#L441-L457: leave an existing profile version unchanged; create a governed successor version.packages/database/src/seed.ts#L397-L407: apply the same append-only policy-version behavior.packages/database/src/seed.ts#L518-L534: apply the same append-only profile-version behavior.
📍 Affects 2 files
packages/database/src/bootstrap.ts#L314-L324(this comment)packages/database/src/bootstrap.ts#L441-L457packages/database/src/seed.ts#L397-L407packages/database/src/seed.ts#L518-L534
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/src/bootstrap.ts` around lines 314 - 324, Make policy and
profile reseeding append-only: in packages/database/src/bootstrap.ts lines
314-324 and 441-457, and packages/database/src/seed.ts lines 397-407 and
518-534, stop updating existing version-1 records through the conflict handlers.
Preserve existing immutable versions and create governed successor versions when
policy documents or profile content changes, including the corresponding version
and audit/hash relationships.
Comment on lines
+398
to
+411
| const contentHash = computeProfileContentHash({ | ||
| displayName: seedProfile.displayName, | ||
| description: seedProfile.description, | ||
| role: seedProfile.role, | ||
| operatingInstructions: seedProfile.operatingInstructions, | ||
| communicationStyle: seedProfile.communicationStyle, | ||
| examplePrompts: seedProfile.examplePrompts, | ||
| modelPolicyId: seedProfile.modelPolicyId, | ||
| memoryPolicyId: null, | ||
| toolPolicyId: null, | ||
| escalationPolicyId: null, | ||
| skillIds: [], | ||
| channelPolicy, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the canonical profile hash payload.
prepareProfileProposal hashes avatarAssetId, including its default null; these seed paths omit it. The same logical profile therefore receives a different content hash depending on creation path.
packages/database/src/bootstrap.ts#L398-L411: includeavatarAssetId: null, preferably by reusing a shared canonical hash helper.packages/database/src/seed.ts#L475-L488: use that same canonical helper/payload.
📍 Affects 2 files
packages/database/src/bootstrap.ts#L398-L411(this comment)packages/database/src/seed.ts#L475-L488
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/src/bootstrap.ts` around lines 398 - 411, Use the canonical
profile hash payload for both seed paths: update computeProfileContentHash usage
in packages/database/src/bootstrap.ts at lines 398-411 and
packages/database/src/seed.ts at lines 475-488 to include avatarAssetId: null,
preferably by reusing the shared helper or payload used by
prepareProfileProposal. Keep all other profile fields unchanged so equivalent
profiles produce identical hashes.
Comment on lines
+413
to
+440
| await db | ||
| .insert(schema.agentProfileVersions) | ||
| .values({ | ||
| id: seedProfile.id, | ||
| organisationId: starterIds.organisation, | ||
| agentId: seedProfile.agentId, | ||
| version: 1, | ||
| basedOnVersionId: null, | ||
| displayName: seedProfile.displayName, | ||
| description: seedProfile.description, | ||
| role: seedProfile.role, | ||
| operatingInstructions: seedProfile.operatingInstructions, | ||
| communicationStyle: seedProfile.communicationStyle, | ||
| examplePrompts: seedProfile.examplePrompts, | ||
| modelPolicyId: seedProfile.modelPolicyId, | ||
| memoryPolicyId: null, | ||
| toolPolicyId: null, | ||
| escalationPolicyId: null, | ||
| skillIds: [], | ||
| channelPolicy, | ||
| contentHash, | ||
| changeRationale: "Initial governed profile established at bootstrap.", | ||
| state: "active", | ||
| createdByActorId: starterIds.actors.system, | ||
| approvedByActorId: starterIds.actors.jordan, | ||
| approvedAt: sql`now()`, | ||
| activatedAt: sql`now()`, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Seeded profiles bypass the required evaluation gate.
Both paths write profiles directly as active, but neither creates a passing agent_profile_evaluations record. Seed via the same evaluated/approved lifecycle, or add an explicit, audited bootstrap exemption.
packages/database/src/bootstrap.ts#L413-L440: persist a passing evaluation before approval/activation.packages/database/src/seed.ts#L490-L517: persist a passing evaluation before approval/activation.
📍 Affects 2 files
packages/database/src/bootstrap.ts#L413-L440(this comment)packages/database/src/seed.ts#L490-L517
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/src/bootstrap.ts` around lines 413 - 440, Update the seeded
profile lifecycle so both packages/database/src/bootstrap.ts lines 413-440 and
packages/database/src/seed.ts lines 490-517 persist a passing
agent_profile_evaluations record before approving or activating the profile,
then retain the active state only after that evaluation gate is satisfied;
alternatively, implement an explicit audited bootstrap exemption consistently in
both sites.
10 tasks
Owner
Author
|
Closing without merge because the product pivot moves profiles and skills to Hermes and removes the end-user/admin UI scope from this change. The branch is retained for selective salvage of server-side policy versioning and approval patterns under #73. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #73's versioned agent profile model against Muster's existing (already-substantial) agent-skill governance foundation, reusing its DB, audit, outbox, and approval patterns rather than duplicating them.
agent_profile_versions(new table): draft → approved → active → retired, one row per version,(agentId, version)unique, partial-unique index enforcing exactly oneactiveversion per agent. Self-approval is rejected both by the service layer and by a DBCHECKconstraint (approved_by_actor_id <> created_by_actor_id) — an approver can never be the actor who proposed the change.agent_policies(new table): versionedmodel/memory/tool/escalationpolicy documents referenced by profile versions.agent_profile_evaluations(new table): mirrors the existing skill-evaluation gate — a profile version cannot be approved without a passing evaluation (score ≥ 80, no regressions, no unsafe-instruction patterns).agent_runs.agent_profile_version_id(new column): every run now records the exact, immutable profile-version snapshot it used — historical runs stay explainable even after the profile is later rolled back or superseded. Wired into both run-creation paths (agent-harnessandagent-gateway).agent-harness'sinvoke(): the kill-switch check now re-runs inside the same transaction as the run insert (SELECT ... FOR UPDATE), not just before it.apps/web/lib/agent-profile-domain.ts) + API route (/api/v1/agents/[id]/profile), mirroring the existingagent-learning-domain.ts//learningroute exactly: transactional audit + outbox writes, org-scoped queries throughout, propose/evaluate/approve/activate/reject/rollback/retire actions, pluscreate_policy.agents.manage) only ever receive the active version's public fields (name, role, communication style, example prompts) — never drafts, operating instructions, or policy internals. This is enforced in the API response shape itself, not just hidden in the UI.bootstrap.tsandseed.ts, matching the issue's specified capabilities/restrictions per agent.GovernedProfilePanel) — version cards with state badges, evaluation score/regressions, and the full evaluate/approve/reject/activate/roll back/retire action set, mirroring the existing skill-proposal panel's UX exactly.What was already there (not duplicated)
Investigation found the skill lifecycle (draft → evaluating → published → rolled_back, with evaluation gating, kill switch, and audit trail) already fully implemented in
agent-learning-domain.ts/packages/agents/src/index.ts. This PR reuses that pattern for profiles rather than reinventing it, and leaves the skill tables/flow untouched.Validation
All commands run locally against a real Postgres + Redis (via Docker), migration order matching CI exactly (
migrate → bootstrap → verify-clean → lint → typecheck → test:unit → build → playwright):New regression coverage:
packages/agents/src/profile-governance.test.ts— pure governance functions (hash determinism, unsafe-pattern rejection, self-approval rejection, activation-state gating).apps/web/lib/agent-profile-domain.integration.test.ts— 9 cases: happy-path propose→evaluate→approve→activate, self-approval rejected at both app and DB layers, activation requires "approved" state, exactly one active version per agent, rollback restores predecessor, historical run attribution survives later changes, retire blocked while active, cross-tenant isolation, policy versioning.tests/agents-admin.spec.ts— 3 Playwright specs against the full stack: agent directory shows Alfie/Jessie/Parker distinctly, admin can propose+evaluate a profile version from the Versions tab, overview shows example prompts/availability.Migration / rollback notes
Additive-only migration (
0021_last_loki.sql): 3 new tables, 2 new nullable columns (agent_definitions.active_profile_version_id,agent_runs.agent_profile_version_id), no data rewrites, no backward-incompatible changes. Safe to roll back by reverting the migration; existing skill/run/agent behavior is unaffected since nothing existing was altered, only extended.Residual risks
agent_policies.document) are freeform JSON with no per-kindschema validation yet — acceptable for this pass since only a minimalmodelpolicy is seeded, but a future issue should addkind-specific Zod schemas if policies grow more complex.propose_profileaction) and exercised by the new Playwright spec via direct API call; a dedicated authoring UI is a reasonable follow-up.ApprovalDomainService.decide(used by unrelated integration-action/hunt/report approvals) still lacks a self-approval guard — out of scope for Publish versioned Muster skill packs and policy bundles for Hermes #73 but flagged during review as a pre-existing gap worth a follow-up issue.Closes #73
Summary by CodeRabbit
New Features
Bug Fixes