Phase 4A: Add controlled operator-approved memory persistence#99
Open
besfeng23 wants to merge 1 commit into
Open
Phase 4A: Add controlled operator-approved memory persistence#99besfeng23 wants to merge 1 commit into
besfeng23 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a controlled memory-proposal flow for admins: a new proposal table and persistence function, server-side proposal services and actions, admin pages for listing, creation, review, and persistence, plus phase-4a documentation, readiness notes, and unit tests. ChangesPhase 4A memory proposals
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin reviewer
participant NewPage as app/admin/memory/proposals/new/page.tsx
participant CreateAction as createProposalAction
participant ProposalService as createMemoryProposal
participant ProposalTable as public.memory_proposals
Admin->>NewPage: submit proposal form
NewPage->>CreateAction: FormData
CreateAction->>ProposalService: createMemoryProposal(...)
ProposalService->>ProposalTable: insert pending proposal + audit
CreateAction->>Admin: redirect to /app/admin/memory/proposals/[id]
sequenceDiagram
participant Admin as Admin reviewer
participant DetailPage as app/admin/memory/proposals/[id]/page.tsx
participant ProposalActions as app/admin/memory/proposals/actions.ts
participant ProposalService as persistApprovedMemoryProposal
participant Rpc as public.memory_persist_approved_proposal
participant MemoryTables as public.memory_items/public.audit_logs
Admin->>DetailPage: submit review or persist form
DetailPage->>ProposalActions: action request
ProposalActions->>ProposalService: approve/reject/revise/persist
ProposalService->>Rpc: persist approved proposal
Rpc->>MemoryTables: write persisted memory and audit rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc435cfcdb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function audit(client: Client, p: MemoryProposal, action: string, reviewer: string, extra: Record<string, unknown> = {}) { | ||
| return client.from("audit_logs").insert({ user_id: p.user_id, namespace: p.namespace, action, table_name: "memory_proposals", record_id: p.id, after_snapshot: { status: p.status, ...extra }, metadata: { proposalId: p.id, reviewer, phase: "4A", appendOnly: true } }).select("*").single(); | ||
| } | ||
| export async function createMemoryProposal(client: Client, input: ProposalInput, session: PandoraServerSessionResult, runtime: PandoraRuntimeSafetyConfigResult): Promise<Result<MemoryProposal>> { const pf = preflight(session, runtime, input.namespace, true); if (!pf.ok) return pf; if (!input.memory_text?.trim()) return { ok: false, code: "empty_memory_text", error: "memory_text must not be empty." }; const row = { ...input, memory_text: input.memory_text.trim(), user_id: pf.data.userId, proposed_by: pf.data.userId, status: "pending" }; const res = await client.from("memory_proposals").insert(row).select("*").single(); if (res.error || !res.data) return { ok: false, code: "write_error", error: res.error?.message ?? "Proposal insert failed." }; await audit(client, res.data, "proposal_created", pf.data.userId); return { ok: true, data: res.data }; } |
There was a problem hiding this comment.
When audit_logs rejects the insert, Supabase returns { error } rather than throwing, but this path ignores that result and still returns success after creating the proposal; the same pattern is used for review transitions. I checked the migrations and audit_logs has RLS enabled with no insert policy, so in the current authenticated public-key server client these lifecycle writes can succeed without the required audit proof.
Useful? React with 👍 / 👎.
Comment on lines
+74
to
+81
| insert into public.memory_items (id,user_id,namespace,memory_type,title,body,confidence,source_summary,metadata,created_at,updated_at) | ||
| values (v_memory_item_id,v_user_id,v_proposal.namespace::public.pandora_namespace,v_memory_type,coalesce(v_proposal.title,left(v_proposal.memory_text,120)),v_proposal.memory_text,coalesce(v_proposal.confidence,0.5),v_proposal.source_ref,jsonb_build_object('proposalId',v_proposal.id,'proposedBy',v_proposal.proposed_by,'reviewedBy',v_proposal.reviewed_by,'reviewedAt',v_proposal.reviewed_at,'phase','4A','appendOnly',true),v_now,v_now); | ||
| insert into public.memory_sources (id,user_id,namespace,memory_item_id,source_type,source_ref,excerpt,confidence,metadata,created_at) | ||
| values (v_source_id,v_user_id,v_proposal.namespace::public.pandora_namespace,v_memory_item_id,v_source_type,v_proposal.source_ref,v_proposal.memory_text,coalesce(v_proposal.confidence,0.5),jsonb_build_object('proposalId',v_proposal.id,'appendOnly',true),v_now); | ||
| insert into public.memory_patches (id,user_id,namespace,memory_item_id,patch_type,reason,before_snapshot,after_snapshot,metadata,created_at) | ||
| values (v_patch_id,v_user_id,v_proposal.namespace::public.pandora_namespace,v_memory_item_id,'append','phase_4a_approved_proposal',null,jsonb_build_object('body',v_proposal.memory_text,'namespace',v_proposal.namespace),jsonb_build_object('proposalId',v_proposal.id,'appendOnly',true),v_now); | ||
| insert into public.audit_logs (id,user_id,namespace,action,table_name,record_id,before_snapshot,after_snapshot,metadata,created_at) | ||
| values (v_audit_id,v_user_id,v_proposal.namespace::public.pandora_namespace,'proposal_persisted','memory_proposals',v_proposal.id,null,jsonb_build_object('memoryItemId',v_memory_item_id),jsonb_build_object('proposalId',v_proposal.id,'sourceId',v_source_id,'patchId',v_patch_id,'appendOnly',true),v_now); |
There was a problem hiding this comment.
This RPC is security invoker, and I checked the existing migrations: memory_items, memory_sources, memory_patches, and audit_logs have RLS enabled but no authenticated insert policies. When an operator clicks “Persist approved proposal”, the function will reach these inserts as the authenticated user and be rejected by RLS, so Phase 4A persistence cannot actually append the memory/source/patch/audit rows.
Useful? React with 👍 / 👎.
Comment on lines
+37
to
+40
| create policy "memory_proposals_authenticated_update_own_namespace" | ||
| on public.memory_proposals for update to authenticated | ||
| using (auth.uid() = user_id) | ||
| with check (auth.uid() = user_id and namespace in ('real_life','au')); |
There was a problem hiding this comment.
This policy lets any authenticated owner update any column on their proposal, including status, reviewed_by, and persisted_memory_id, without the server-side admin/capability preflight or the reviewed persistence gate. In a normal Supabase deployment with the public client available, that means proposal approval state is not operator-only at the database boundary, so direct clients can manufacture reviewed-looking rows.
Useful? React with 👍 / 👎.
Comment on lines
+67
to
+69
| if v_proposal.memory_type in ('observation','user_preference','soft_canon','hard_canon','contradiction','retcon_candidate','real_life_fact','business_fact','relationship_signal','risk_signal') then | ||
| v_memory_type := v_proposal.memory_type::public.memory_type; | ||
| end if; |
There was a problem hiding this comment.
Because proposals store memory_type as free-form text and the form accepts arbitrary input, this allow-list persists AU-only types such as soft_canon/hard_canon into real_life rows and real-life types such as business_fact into au rows. The existing validators split allowed types by namespace, so approving one of these proposals breaks the namespace separation rules instead of rejecting it before append.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@app/admin/memory/proposals/page.tsx`:
- Line 11: The admin memory proposals page renders the full admin shell before
verifying authentication, so add an authorization guard in Page after
resolvePandoraServerSession() and before any UI is returned. Use the session
result from resolvePandoraServerSession() to short-circuit unauthenticated users
with a redirect or forbidden/unauthorized component, or move the check into an
app/admin layout guard so Page only runs for authorized sessions. Keep the guard
tied to the existing Page and session.ok/session.error handling so the admin UI,
filters, and proposal list are never exposed to unauthenticated users.
In `@lib/auth/pandora-server-session-resolver.ts`:
- Line 21: The session derivation in PandoraServerSessionResolver currently
checks adminCapabilities against app_metadata.roles but does not apply the same
array fallback when computing isInternalOperator and isPersistenceOperator.
Update the session object construction so the operator flags in the
PandoraServerSession derived from user.app_metadata also consider roles arrays
(for example alongside role and explicit booleans), keeping the logic consistent
with the existing adminCapabilities fallback.
In `@lib/services/memory-proposal-service.ts`:
- Around line 27-30: The transition() helper in memory-proposal-service.ts only
rejects persisted and disabled states, so reviewed proposals can still be moved
between approved, rejected, and needs_revision via approveMemoryProposal,
rejectMemoryProposal, and requestMemoryProposalRevision. Update transition() to
enforce the full review state machine by allowing status changes only from
pending into a review outcome and blocking any transition from already reviewed
or terminal statuses, keeping the guard centralized in transition().
- Around line 21-24: The createMemoryProposal flow and the audit() helper
currently perform separate best-effort writes, so a proposal can succeed even if
the audit_logs insert fails. Refactor memory-proposal-service.ts so
createMemoryProposal (and any other callers that use audit()) uses a single
transactional DB operation or RPC that writes the proposal mutation and audit
record atomically, and make the caller fail if the audit write does not succeed.
Keep the existing audit() and createMemoryProposal symbols easy to locate when
updating the flow.
In `@supabase/migrations/20260626040000_phase_4a_memory_proposals.sql`:
- Around line 33-40: The Phase 4A write path is still bypassable because the
`memory_proposals_authenticated_insert_own_namespace` and
`memory_proposals_authenticated_update_own_namespace` policies let a normal
authenticated user create and mutate proposal state directly, which then allows
`memory_persist_approved_proposal()` to trust a forged `approved` row and write
active-memory tables. Tighten the database boundary by changing the
`public.memory_proposals` policies to keep direct writes pending-only or
read-only, and ensure the approval/persistence transition is only reachable
through a DB-controlled routine that verifies operator/reviewed-write claims
before any insert into `memory_items`, `memory_sources`, `memory_patches`, or
`audit_logs`. Also revoke broad `UPDATE` and `EXECUTE` access so the reviewed
gate is enforced in the database, not only in the TS service.
In `@tests/unit/memory-proposal-service.test.ts`:
- Line 18: Tighten the mock in the test helper’s single() implementation so it
only returns a record when the filters match exactly. Remove the store.at(-1)
fallback and return a “not found” error when no item satisfies
filters.every(...), so the assertions around MemoryProposalService queries
continue to verify the id/user_id contract.
- Around line 41-44: The test around persistApprovedMemoryProposal only checks
the ok flag, so it can miss cases where the proposal is not actually moved to
persisted. Update the assertions in memory-proposal-service.test.ts to verify
the refreshed proposal state after the first call, including that the status is
persisted and persisted_memory_id is set, and keep the existing audit check plus
the second-call no-op expectation to catch regressions in
persistApprovedMemoryProposal.
🪄 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
Run ID: 27d90f82-773c-4200-8f97-09c11af06265
📒 Files selected for processing (11)
app/admin/memory/proposals/[id]/page.tsxapp/admin/memory/proposals/actions.tsapp/admin/memory/proposals/new/page.tsxapp/admin/memory/proposals/page.tsxapp/admin/memory/verification/page.tsxdocs/phase-4a-controlled-memory-persistence.mddocs/templates/memory-proposal-approval-proof.mdlib/auth/pandora-server-session-resolver.tslib/services/memory-proposal-service.tssupabase/migrations/20260626040000_phase_4a_memory_proposals.sqltests/unit/memory-proposal-service.test.ts
| import { listMemoryProposals } from "@/lib/services/memory-proposal-service"; | ||
| export const dynamic = "force-dynamic"; | ||
| type Props = { searchParams?: Promise<Record<string,string|undefined>> }; | ||
| export default async function Page({ searchParams }: Props) { const p = await searchParams; const namespace = p?.namespace ?? "real_life"; const status = p?.status; const session = await resolvePandoraServerSession(); const runtime = resolvePandoraRuntimeSafetyConfig(); const supabase = await createSupabaseServerClient() as never; const proposals = await listMemoryProposals(supabase, namespace, status, session, runtime); return <AppShell><PageHeader eyebrow="Internal admin" title="Memory proposals" description="Controlled proposal → review → approve → persist → audit workflow. Public writes remain disabled. Model calls, embeddings, retrieval, GPT Actions, and MCP remain disabled."/><SectionCard title="Phase 4A gate" description="This does not write active memory until approved. Approval is audited."><p>Reviewed persistence gate enabled: {String(runtime.config.approvedReviewPersistenceEnabled)}</p><p>Public writes remain disabled: {String(!runtime.config.publicMemoryPersistenceEnabled)}</p><Link className="button-link button-link--primary" href="/admin/memory/proposals/new">New proposal</Link></SectionCard><SectionCard title="Filters" description="Namespace-scoped proposal list."><form><select name="namespace" defaultValue={namespace}><option value="real_life">real_life</option><option value="au">au</option></select><select name="status" defaultValue={status ?? ""}><option value="">all</option>{["pending","approved","rejected","needs_revision","persisted","disabled"].map(s=><option key={s} value={s}>{s}</option>)}</select><button>Apply</button></form>{!proposals.ok ? <p>{proposals.error}</p> : <ul>{proposals.data.map(x => <li key={x.id}><Link href={`/admin/memory/proposals/${x.id}`}>{x.title ?? x.id}</Link> — {x.status} — {x.namespace}</li>)}</ul>}</SectionCard></AppShell> } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/admin layouts with auth/redirect logic =="
fd -a 'layout.(ts|tsx)' app/admin | xargs -r rg -n -C3 'resolvePandoraServerSession|redirect\(|forbidden\(|unauthorized\(|notFound\('
echo "== middleware touching /admin routes =="
fd -a 'middleware.(ts|tsx|js|mjs)' . | xargs -r rg -n -C3 '/admin|redirect\(|forbidden\(|unauthorized\('
echo "== local page body =="
sed -n '1,160p' app/admin/memory/proposals/page.tsxRepository: besfeng23/Memory
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking directory structure..."
ls -la app/admin/ 2>/dev/null || echo "app/admin not found or empty"
echo "Checking for layout files..."
ls -la app/admin/layout.* 2>/dev/null || echo "No layout files in app/admin"
echo "Checking for middleware..."
ls -la middleware.* 2>/dev/null || echo "No middleware file in root"Repository: besfeng23/Memory
Length of output: 501
🏁 Script executed:
cat middleware.tsRepository: besfeng23/Memory
Length of output: 1520
Missing authentication guard exposes admin UI to unauthenticated users.
middleware.ts calls await supabase.auth.getUser() but unconditionally returns NextResponse.next() without checking the result or redirecting unauthorized users. Additionally, app/admin lacks a layout.tsx guard. Consequently, the Page component renders the admin shell (headers, CTAs, filters) before verifying session.ok.
Wrap the return value in if (!session.ok) return <ForbiddenOrRedirect />; or add a layout guard that redirects if session.error is present.
// app/admin/memory/proposals/page.tsx
if (!session.ok) {
return <NotAuthorizedOrRedirect />; // or redirect('/login')
}🤖 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 `@app/admin/memory/proposals/page.tsx` at line 11, The admin memory proposals
page renders the full admin shell before verifying authentication, so add an
authorization guard in Page after resolvePandoraServerSession() and before any
UI is returned. Use the session result from resolvePandoraServerSession() to
short-circuit unauthenticated users with a redirect or forbidden/unauthorized
component, or move the check into an app/admin layout guard so Page only runs
for authorized sessions. Keep the guard tied to the existing Page and
session.ok/session.error handling so the admin UI, filters, and proposal list
are never exposed to unauthenticated users.
| const user = await getCurrentUser(); | ||
| if (!user?.id) return { ok: false, session: null, blockers: [{ code: "auth_required", message: "Authenticated server session is required." }, { code: "session_unavailable", message: "No real auth provider session was resolved; no production user is faked." }] }; | ||
| const session: PandoraServerSession = { userId: user.id, email: user.email, displayName: user.user_metadata?.name ?? user.user_metadata?.full_name, authProvider: user.app_metadata?.provider, authenticated: true, allowedNamespaces: ["real_life", "au"], adminCapabilities: [], isInternalOperator: false, isPersistenceOperator: false, sessionSource: "supabase", serverDerivedOnly: true, clientUserIdAccepted: false, publicReadAllowed: false, publicPersistenceAllowed: false, serviceRoleUsed: false }; | ||
| const session: PandoraServerSession = { userId: user.id, email: user.email, displayName: user.user_metadata?.name ?? user.user_metadata?.full_name, authProvider: user.app_metadata?.provider, authenticated: true, allowedNamespaces: ["real_life", "au"], adminCapabilities: Array.isArray(user.app_metadata?.adminCapabilities) ? user.app_metadata.adminCapabilities : Array.isArray(user.app_metadata?.roles) ? user.app_metadata.roles : [], isInternalOperator: user.app_metadata?.role === "admin" || user.app_metadata?.role === "operator" || user.app_metadata?.isInternalOperator === true, isPersistenceOperator: user.app_metadata?.isPersistenceOperator === true || user.app_metadata?.role === "persistence_operator", sessionSource: "supabase", serverDerivedOnly: true, clientUserIdAccepted: false, publicReadAllowed: false, publicPersistenceAllowed: false, serviceRoleUsed: false }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include array-based roles when deriving operator flags.
adminCapabilities already falls back to app_metadata.roles, but isInternalOperator / isPersistenceOperator ignore that same array. A session shaped like { roles: ["admin"] } or { roles: ["persistence_operator"] } will still resolve to false here, so the new proposal preflights reject legitimate operators.
Suggested fix
+ const roles = Array.isArray(user.app_metadata?.roles)
+ ? user.app_metadata.roles.filter((role): role is string => typeof role === "string")
+ : [];
+ const adminCapabilities = Array.isArray(user.app_metadata?.adminCapabilities)
+ ? user.app_metadata.adminCapabilities.filter((cap): cap is string => typeof cap === "string")
+ : roles;
- const session: PandoraServerSession = { userId: user.id, email: user.email, displayName: user.user_metadata?.name ?? user.user_metadata?.full_name, authProvider: user.app_metadata?.provider, authenticated: true, allowedNamespaces: ["real_life", "au"], adminCapabilities: Array.isArray(user.app_metadata?.adminCapabilities) ? user.app_metadata.adminCapabilities : Array.isArray(user.app_metadata?.roles) ? user.app_metadata.roles : [], isInternalOperator: user.app_metadata?.role === "admin" || user.app_metadata?.role === "operator" || user.app_metadata?.isInternalOperator === true, isPersistenceOperator: user.app_metadata?.isPersistenceOperator === true || user.app_metadata?.role === "persistence_operator", sessionSource: "supabase", serverDerivedOnly: true, clientUserIdAccepted: false, publicReadAllowed: false, publicPersistenceAllowed: false, serviceRoleUsed: false };
+ const session: PandoraServerSession = { userId: user.id, email: user.email, displayName: user.user_metadata?.name ?? user.user_metadata?.full_name, authProvider: user.app_metadata?.provider, authenticated: true, allowedNamespaces: ["real_life", "au"], adminCapabilities, isInternalOperator: roles.includes("admin") || roles.includes("operator") || user.app_metadata?.role === "admin" || user.app_metadata?.role === "operator" || user.app_metadata?.isInternalOperator === true, isPersistenceOperator: roles.includes("persistence_operator") || user.app_metadata?.isPersistenceOperator === true || user.app_metadata?.role === "persistence_operator", sessionSource: "supabase", serverDerivedOnly: true, clientUserIdAccepted: false, publicReadAllowed: false, publicPersistenceAllowed: false, serviceRoleUsed: false };📝 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 session: PandoraServerSession = { userId: user.id, email: user.email, displayName: user.user_metadata?.name ?? user.user_metadata?.full_name, authProvider: user.app_metadata?.provider, authenticated: true, allowedNamespaces: ["real_life", "au"], adminCapabilities: Array.isArray(user.app_metadata?.adminCapabilities) ? user.app_metadata.adminCapabilities : Array.isArray(user.app_metadata?.roles) ? user.app_metadata.roles : [], isInternalOperator: user.app_metadata?.role === "admin" || user.app_metadata?.role === "operator" || user.app_metadata?.isInternalOperator === true, isPersistenceOperator: user.app_metadata?.isPersistenceOperator === true || user.app_metadata?.role === "persistence_operator", sessionSource: "supabase", serverDerivedOnly: true, clientUserIdAccepted: false, publicReadAllowed: false, publicPersistenceAllowed: false, serviceRoleUsed: false }; | |
| const roles = Array.isArray(user.app_metadata?.roles) | |
| ? user.app_metadata.roles.filter((role): role is string => typeof role === "string") | |
| : []; | |
| const adminCapabilities = Array.isArray(user.app_metadata?.adminCapabilities) | |
| ? user.app_metadata.adminCapabilities.filter((cap): cap is string => typeof cap === "string") | |
| : roles; | |
| const session: PandoraServerSession = { userId: user.id, email: user.email, displayName: user.user_metadata?.name ?? user.user_metadata?.full_name, authProvider: user.app_metadata?.provider, authenticated: true, allowedNamespaces: ["real_life", "au"], adminCapabilities, isInternalOperator: roles.includes("admin") || roles.includes("operator") || user.app_metadata?.role === "admin" || user.app_metadata?.role === "operator" || user.app_metadata?.isInternalOperator === true, isPersistenceOperator: roles.includes("persistence_operator") || user.app_metadata?.isPersistenceOperator === true || user.app_metadata?.role === "persistence_operator", sessionSource: "supabase", serverDerivedOnly: true, clientUserIdAccepted: false, publicReadAllowed: false, publicPersistenceAllowed: false, serviceRoleUsed: false }; |
🤖 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 `@lib/auth/pandora-server-session-resolver.ts` at line 21, The session
derivation in PandoraServerSessionResolver currently checks adminCapabilities
against app_metadata.roles but does not apply the same array fallback when
computing isInternalOperator and isPersistenceOperator. Update the session
object construction so the operator flags in the PandoraServerSession derived
from user.app_metadata also consider roles arrays (for example alongside role
and explicit booleans), keeping the logic consistent with the existing
adminCapabilities fallback.
Comment on lines
+21
to
+24
| async function audit(client: Client, p: MemoryProposal, action: string, reviewer: string, extra: Record<string, unknown> = {}) { | ||
| return client.from("audit_logs").insert({ user_id: p.user_id, namespace: p.namespace, action, table_name: "memory_proposals", record_id: p.id, after_snapshot: { status: p.status, ...extra }, metadata: { proposalId: p.id, reviewer, phase: "4A", appendOnly: true } }).select("*").single(); | ||
| } | ||
| export async function createMemoryProposal(client: Client, input: ProposalInput, session: PandoraServerSessionResult, runtime: PandoraRuntimeSafetyConfigResult): Promise<Result<MemoryProposal>> { const pf = preflight(session, runtime, input.namespace, true); if (!pf.ok) return pf; if (!input.memory_text?.trim()) return { ok: false, code: "empty_memory_text", error: "memory_text must not be empty." }; const row = { ...input, memory_text: input.memory_text.trim(), user_id: pf.data.userId, proposed_by: pf.data.userId, status: "pending" }; const res = await client.from("memory_proposals").insert(row).select("*").single(); if (res.error || !res.data) return { ok: false, code: "write_error", error: res.error?.message ?? "Proposal insert failed." }; await audit(client, res.data, "proposal_created", pf.data.userId); return { ok: true, data: res.data }; } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the proposal mutation and audit write atomic.
Line 24 and Line 27 both await audit(), but neither caller checks whether the audit_logs insert succeeded. A proposal can therefore be created or transitioned successfully without the append-only proof this Phase 4A flow is supposed to guarantee. This should be one transactional database operation/RPC, not two best-effort writes.
Also applies to: 27-27
🤖 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 `@lib/services/memory-proposal-service.ts` around lines 21 - 24, The
createMemoryProposal flow and the audit() helper currently perform separate
best-effort writes, so a proposal can succeed even if the audit_logs insert
fails. Refactor memory-proposal-service.ts so createMemoryProposal (and any
other callers that use audit()) uses a single transactional DB operation or RPC
that writes the proposal mutation and audit record atomically, and make the
caller fail if the audit write does not succeed. Keep the existing audit() and
createMemoryProposal symbols easy to locate when updating the flow.
Comment on lines
+27
to
+30
| async function transition(client: Client, id: string, status: MemoryProposalStatus, session: PandoraServerSessionResult, runtime: PandoraRuntimeSafetyConfigResult, fields: Record<string, unknown> = {}) { const pf = preflight(session, runtime, undefined, true); if (!pf.ok) return pf; const cur = await getMemoryProposal(client, id, session, runtime); if (!cur.ok) return cur; if (["persisted", "disabled"].includes(cur.data.status)) return { ok: false as const, code: "terminal_status", error: "Terminal proposals cannot transition." }; const reviewed_at = new Date().toISOString(); const res = await client.from("memory_proposals").update({ status, reviewed_by: pf.data.userId, reviewed_at, updated_at: reviewed_at, ...fields }).eq("id", id).eq("user_id", pf.data.userId).select("*").single(); if (res.error || !res.data) return { ok: false as const, code: "write_error", error: res.error?.message ?? "Transition failed." }; await audit(client, res.data, `proposal_${status}`, pf.data.userId, fields); return { ok: true as const, data: res.data }; } | ||
| export const approveMemoryProposal = (client: Client, id: string, reviewer: string, session: PandoraServerSessionResult, runtime: PandoraRuntimeSafetyConfigResult) => transition(client, id, "approved", session, runtime, { reviewer }); | ||
| export const rejectMemoryProposal = (client: Client, id: string, reason: string, reviewer: string, session: PandoraServerSessionResult, runtime: PandoraRuntimeSafetyConfigResult) => transition(client, id, "rejected", session, runtime, { rejection_reason: reason, reviewer }); | ||
| export const requestMemoryProposalRevision = (client: Client, id: string, note: string, reviewer: string, session: PandoraServerSessionResult, runtime: PandoraRuntimeSafetyConfigResult) => transition(client, id, "needs_revision", session, runtime, { revision_note: note, reviewer }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the review state machine in transition().
This helper only blocks persisted and disabled, so a crafted POST can still approve, reject, or revise proposals that are already reviewed. The detail page disables those buttons unless the status is pending, but the exported server actions call this function directly, so rejected / needs_revision proposals can be re-approved and later persisted.
🤖 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 `@lib/services/memory-proposal-service.ts` around lines 27 - 30, The
transition() helper in memory-proposal-service.ts only rejects persisted and
disabled states, so reviewed proposals can still be moved between approved,
rejected, and needs_revision via approveMemoryProposal, rejectMemoryProposal,
and requestMemoryProposalRevision. Update transition() to enforce the full
review state machine by allowing status changes only from pending into a review
outcome and blocking any transition from already reviewed or terminal statuses,
keeping the guard centralized in transition().
Comment on lines
+33
to
+40
| create policy "memory_proposals_authenticated_insert_own_namespace" | ||
| on public.memory_proposals for insert to authenticated | ||
| with check (auth.uid() = user_id and auth.uid() = proposed_by and status = 'pending' and reviewed_by is null and persisted_memory_id is null); | ||
|
|
||
| create policy "memory_proposals_authenticated_update_own_namespace" | ||
| on public.memory_proposals for update to authenticated | ||
| using (auth.uid() = user_id) | ||
| with check (auth.uid() = user_id and namespace in ('real_life','au')); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Lock down the DB write path to reviewed operators only.
These policies plus the invoker RPC make the Phase 4A boundary bypassable with a normal authenticated JWT. A row owner can insert their own proposal, update status/review fields directly under Lines 37-40, then call memory_persist_approved_proposal() because the function only trusts auth.uid() and status = 'approved' before writing memory_items, memory_sources, memory_patches, and audit_logs. That means the reviewed runtime gate and admin approval flow are enforced only in the TS service, not at the actual database boundary.
Please move the approval/persist transition behind a DB-enforced privilege boundary as well: keep direct table writes pending-only (or read-only), revoke broad UPDATE/EXECUTE access, and perform approval/persistence through routines that validate operator claims and the reviewed-write gate before any active-memory insert happens.
Also applies to: 42-83
🤖 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 `@supabase/migrations/20260626040000_phase_4a_memory_proposals.sql` around
lines 33 - 40, The Phase 4A write path is still bypassable because the
`memory_proposals_authenticated_insert_own_namespace` and
`memory_proposals_authenticated_update_own_namespace` policies let a normal
authenticated user create and mutate proposal state directly, which then allows
`memory_persist_approved_proposal()` to trust a forged `approved` row and write
active-memory tables. Tighten the database boundary by changing the
`public.memory_proposals` policies to keep direct writes pending-only or
read-only, and ensure the approval/persistence transition is only reachable
through a DB-controlled routine that verifies operator/reviewed-write claims
before any insert into `memory_items`, `memory_sources`, `memory_patches`, or
`audit_logs`. Also revoke broad `UPDATE` and `EXECUTE` access so the reviewed
gate is enforced in the database, not only in the TS service.
| update(v: any){ pendingUpdate = v; return this; }, | ||
| eq(k: string, v: any){ filters.push([k,v]); return this; }, | ||
| order(){ return this; }, | ||
| async single(){ const store = table === "audit_logs" ? audits : rows; const data = store.find(x => filters.every(([k,v]) => x[k] === v)) ?? store.at(-1); if (data && pendingUpdate) Object.assign(data, pendingUpdate); return { data, error: data ? null : { message: "not found" } }; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the mock's single() behavior.
Falling back to store.at(-1) when no row matches the filters makes these tests pass even if the service stops constraining by id or user_id. Return not found on a filter miss so the auth/query assertions actually protect the query contract.
Suggested test fix
- async single(){ const store = table === "audit_logs" ? audits : rows; const data = store.find(x => filters.every(([k,v]) => x[k] === v)) ?? store.at(-1); if (data && pendingUpdate) Object.assign(data, pendingUpdate); return { data, error: data ? null : { message: "not found" } }; }
+ async single(){
+ const store = table === "audit_logs" ? audits : rows;
+ const data = store.find((x) => filters.every(([k, v]) => x[k] === v));
+ if (!data) return { data: null, error: { message: "not found" } };
+ if (pendingUpdate) Object.assign(data, pendingUpdate);
+ return { data, error: null };
+ }📝 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
| async single(){ const store = table === "audit_logs" ? audits : rows; const data = store.find(x => filters.every(([k,v]) => x[k] === v)) ?? store.at(-1); if (data && pendingUpdate) Object.assign(data, pendingUpdate); return { data, error: data ? null : { message: "not found" } }; } | |
| async single(){ | |
| const store = table === "audit_logs" ? audits : rows; | |
| const data = store.find((x) => filters.every(([k, v]) => x[k] === v)); | |
| if (!data) return { data: null, error: { message: "not found" } }; | |
| if (pendingUpdate) Object.assign(data, pendingUpdate); | |
| return { data, error: null }; | |
| } |
🤖 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/unit/memory-proposal-service.test.ts` at line 18, Tighten the mock in
the test helper’s single() implementation so it only returns a record when the
filters match exactly. Remove the store.at(-1) fallback and return a “not found”
error when no item satisfies filters.every(...), so the assertions around
MemoryProposalService queries continue to verify the id/user_id contract.
Comment on lines
+41
to
+44
| const persisted = await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true)); | ||
| expect(persisted.ok).toBe(true); | ||
| expect(c.audits.some((a: any) => a.action === "proposal_persisted")).toBe(true); | ||
| expect((await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true))).ok).toBe(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the persisted proposal state, not just ok.
This test still passes if persistence returns success without actually leaving the proposal in persisted state. Please assert the refreshed proposal status and persisted_memory_id so RPC no-ops/regressions are caught.
Suggested assertion upgrade
const persisted = await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true));
expect(persisted.ok).toBe(true);
+ expect(persisted.ok && persisted.data.proposal.status).toBe("persisted");
+ expect(persisted.ok && persisted.data.proposal.persisted_memory_id).toBe("00000000-0000-0000-0000-000000000099");
expect(c.audits.some((a: any) => a.action === "proposal_persisted")).toBe(true);📝 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 persisted = await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true)); | |
| expect(persisted.ok).toBe(true); | |
| expect(c.audits.some((a: any) => a.action === "proposal_persisted")).toBe(true); | |
| expect((await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true))).ok).toBe(false); | |
| const persisted = await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true)); | |
| expect(persisted.ok).toBe(true); | |
| expect(persisted.ok && persisted.data.proposal.status).toBe("persisted"); | |
| expect(persisted.ok && persisted.data.proposal.persisted_memory_id).toBe("00000000-0000-0000-0000-000000000099"); | |
| expect(c.audits.some((a: any) => a.action === "proposal_persisted")).toBe(true); | |
| expect((await persistApprovedMemoryProposal(c, created.ok ? created.data.id : "", "reviewer", session(), runtime(true))).ok).toBe(false); |
🤖 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/unit/memory-proposal-service.test.ts` around lines 41 - 44, The test
around persistApprovedMemoryProposal only checks the ok flag, so it can miss
cases where the proposal is not actually moved to persisted. Update the
assertions in memory-proposal-service.test.ts to verify the refreshed proposal
state after the first call, including that the status is persisted and
persisted_memory_id is set, and keep the existing audit check plus the
second-call no-op expectation to catch regressions in
persistApprovedMemoryProposal.
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.
Motivation
Description
supabase/migrations/20260626040000_phase_4a_memory_proposals.sqlthat createspublic.memory_proposals, RLS policies scoped to authenticated user + namespace, and asecurity invokerRPCmemory_persist_approved_proposalthat appends memory item, source, patch, and audit proof for approved proposals.lib/services/memory-proposal-service.tswithcreateMemoryProposal,listMemoryProposals,getMemoryProposal,approveMemoryProposal,rejectMemoryProposal,requestMemoryProposalRevision, andpersistApprovedMemoryProposalenforcing session, admin capability, namespace checks, gate enforcement (PANDORA_ENABLE_APPROVED_REVIEW_MEMORY_PERSISTENCE), input validation, and audit writes.app/admin/memory/proposals(list, new, detail/review) inactions.ts,page.tsx,new/page.tsx, and[id]/page.tsx, with blunt safety copy and gate-disabled UI states; mutations use server actions/POST only and call server Supabase client.app/admin/memory/verification/page.tsxwith a Phase 4A readiness section while preserving Phase 3F read-only closure messaging, adjust server session resolution to derive admin/operator capabilities from Supabase app metadata, and add docsdocs/phase-4a-controlled-memory-persistence.mdand proof templatedocs/templates/memory-proposal-approval-proof.md.tests/unit/memory-proposal-service.test.tscovering auth/gate blocks, empty text rejection, create/approve/reject audit events, approved persistence, double-persist blocking, and rejected-proposal persist blocking.Testing
npm run typecheck, which passed without errors.npm run lint, which passed; remaining pre-existing warnings were noted inlib/db/core-repositories.tsandvitest.config.tsand did not block the change.npm run test, which passed (73 files, 439 tests), and the new unit tests intests/unit/memory-proposal-service.test.tspassed.npm run build, which completed successfully; the build reported an existing Supabase Edge Runtime warning but did not fail.Codex Task
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests