Learning engagement context and Nina harness#187
Conversation
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
nabilfatih
changed the title
![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: 694a878d1d
ℹ️ 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".
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0e6f1b9df
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2534043e4b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9bc537a57
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c5adbdd20
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Persist the current device on cross-device hits
When a signed-in learner first views content on device A and then on device B, this returns the existing user row from device A without recording device B anywhere. If the learner signs out on device B, the auth-aware client key sends an anonymous view, loadExistingView finds no device-B row, and popularity inserts a new device:<id> viewer signal because the anonymous path cannot check the prior user:<id> signal, so one learner can be counted twice for the same content/day.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Avoid reusing another user’s device view
When the same browser/device is used by multiple signed-in accounts, this prefers the existing device row even if it already belongs to a different userId. In that case the later viewer never gets their own learningViews row (the update path won't replace a populated view.userId), and the learningViews trigger attributes the new view event to the previous user; only reuse the device row when it is anonymous or owned by the current user, otherwise fall through to the current user's row or insert a new one.
ℹ️ 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".
|
Resolved the Codex Review body concerns around |
1 participant
Source of truth
Final accepted plan v3:
file:///private/tmp/nakafa-education-agent-harness-final-plan-v3-20260622-1635.htmlLatest pushed SHA:
09d666ca7e688500ad804e1b1a9a364aafefefd6PR state: ready for review, not draft. Title remains exactly
Learning engagement context and Nina harness.Current readiness state
Implementation, local verification, Convex dev/prod deployment, GitHub checks, review-thread cleanup, root-start runtime proof, Browser-plugin proof for unchanged app/UI surfaces, and the latest engagement identity review-body blocker fix are current for PR #187.
PR #187 is not mergeable until required human review approves it:
reviewDecision=REVIEW_REQUIRED.Latest check state at body update time on
09d666ca7e:React Doctor: pass, score 100/100.react-doctor: pass.agent-docs: pass.Thread-aware GraphQL review-thread checks previously returned no unresolved current or outdated inline review threads. The later Codex Review body comments on
packages/backend/convex/contents/views/impl.ts:112are addressed by09d666ca7e; they were body comments on older reviewed commits, not unresolved inline threads.Final-plan-v3 implementation matrix
NinaHarness.stream; route/framework code knows request/auth plus the harness entry point.packages/ai/ninainternals.packages/ai/agents/orchestratorownership was deleted/absorbed intonina/prompt,nina/policy, andnina/capability.NinaHarness.stream; deterministic policy handles simple routing, math truth is evidence-first, and research is reserved for source-heavy/current/external work.Engagement identity blocker handling on 09d666c
Fixed the Codex Review body concerns around
packages/backend/convex/contents/views/impl.ts:112by making thelearningViewscontract explicit and indexed:learningViewsare now documented as one row per anonymous device or authenticated user-device for each canonical asset and verified context.learningViews.by_userId_and_deviceId_and_content_id_and_contextKeyso signed-in requests can load only their own row on the current device.learningViews.by_deviceId_and_content_id_and_contextKey_and_lastViewedAtso unsigned device reads are indexed, bounded, and latest-row aware when multiple accounts have used the same browser/device.user:<id>viewer key.learningViewstrigger does not emit a product analytics event for a previous signed-in user.Regression proof added in
packages/backend/convex/contents/mutations/views.test.ts:learningViewsownership, separateuserLearningRecents, and content-view analytics scheduled for each correct user only.Review-thread handling before 09d666c
Earlier final-plan-v3 blockers remain fixed: pinned Nina context after transcript rewrites, exact-size transcript rewrites, signed-in view keys, paged Continue Learning recents, paged trending counters, atomic transcript replacement, trace retention cron, paged audio popularity reads, and bounded capability trace summaries.
The immediately previous pushed fix on
207a461bdahandled:apps/www/app/api/chat/route.ts:160/ “Resolve pinned context after rewrite deletion”: the route passesmessage.idintoloadPinnedNinaContext; ConvexgetPinnedNinaContextForTurnfinds an existing message by identifier and reads only retained transcript messages usingmessages.by_chatIdwith_creationTime < rewrite point, bounded to 20. This excludes soon-deleted tail snapshots without app-side pre-delete, whilesaveMessageremains the atomic replacement mutation.packages/backend/convex/chats/mutations.ts:145/ “Allow exact-size transcript rewrites”:deleteMessageBatchFromPointreadsCHAT_TRANSCRIPT_REWRITE_MESSAGE_BATCH_SIZE + 1rows as a sentinel, deletes only the supported slice, and reports overflow only when the sentinel exists. Exact 25-message rewrites succeed; 26+ still rejects transactionally through the existingsaveMessageerror path.Verification commands on 09d666c
Passed locally from repo root:
pnpm --filter @repo/backend exec vitest run convex/contents/mutations/views.test.ts convex/triggers/contents/views.test.ts pnpm --filter @repo/backend typecheck pnpm format pnpm lint pnpm build pnpm run doctor --verbose --diff git diff --checkObserved proof:
pnpm build: 17 successful tasks, including full backend tests (145 files / 629 tests), full www tests (82 files / 722 tests), and productionwwwbuild.pnpm run doctor --verbose --diff: 100/100, no changed-source diagnostics.pnpm formatandpnpm lint: passed with the pre-existing Ultracite warning thatpackages/contents/quran/source.tsexceeds the configured max file size.git diff --check: clean before commit.packages/backend/convex/contents/views/impl.tsandpackages/backend/convex/contents/schema.tsfound no raw try/catch, unsafe casts,any, ts-ignore/ts-expect-error, generic thrown expected failures, or Effect runners. The only match was the existingEffect.tryPromisewording in JSDoc.09d666ca7e.Convex deploy and data proof on 09d666c
Dev deployment:
pnpm --filter @repo/backend exec convex dev --onceResult: current Convex functions ready on dev deployment
cheerful-ocelot-306; both newlearningViewsindexes were added.Authorized production deployment:
Result: no indexes deleted, functions uploaded, TypeScript checked, schema validation complete, production functions deployed to
dapper-antelope-269, and both newlearningViewsindexes were added.Dev/prod function-spec verification confirmed the current content-view mutation surface:
Both dev and prod expose
contents/mutations/views.js:recordContentViewafter the current source deploy.No schema/index deletion occurred in the production deploy. This patch adds indexes and changes selection semantics only; it does not delete or overwrite canonical chat messages, user learning evidence, content refs, lifetime counters/checkpoints, eval evidence, or durable engagement facts.
Browser/runtime proof
Fresh root
pnpm startremains running from/Users/nabilfatih/.codex/worktrees/cde1/nakafa.comwithwwwavailable athttp://localhost:3000. Browser proof used the in-app Browser plugin visibly againsthttp://localhost:3000; no external-browser proof was substituted.Latest Browser-plugin proof was completed on
207a461bdabefore the Convex-only engagement identity follow-up. The follow-up commit did not change app UI/chat surfaces, but did receive targeted Convex tests and fresh dev/prod Convex deployment proof on09d666ca7e.Browser-plugin proof details:
http://localhost:3000/id/authshowed the authenticated state withKeluar; no password, MFA, CAPTCHA, or secret handling was attempted./id/homerenderedHi, Nabil Fatih,Lanjutkan Belajar, andMateri Populer Minggu Ini.Hukum Kekekalan Massafrom home and landed on/id/materi/kimia/hukum-dasar-kimia/hukum-kekekalan-massa?ctx=merdeka~class-10-chemistry-basic-chemistry-laws.Hukum Dasar Kimiaback/header link to/id/kurikulum/merdeka/kelas-10/kimia#hukum-dasar-kimia, plusTanya Ninaand page-outline context.ctx=merdeka~class-10-chemistry-basic-chemistry-laws; clicked next tohukum-perbandingan-berganda?ctx=...and clicked previous back tohukum-kekekalan-massa?ctx=..../en/subjects/chemistry/basic-chemistry-laws/mass-conservation-law?ctx=merdeka~class-10-chemistry-basic-chemistry-lawsrendered EnglishMass Conservationcontent./id/curricula/merdekarendered404 - Page Not Found./id/chatacceptedJelaskan hukum kekekalan massa dalam dua kalimat sederhana., created/id/chat/ks79s8mb5s58vtc6f8wfmhv0nx894395, returned a normal answer, and rendered follow-up suggestions includingBerikan satu contoh dalam kehidupan sehari-hari tentang hukum ini.,Apa yang terjadi kalau reaksinya dilakukan di wadah terbuka?, andBisa berikan contoh soal sederhana untuk menghitung massa zat hasil reaksi?.Tanya Ninaand askedDari halaman ini, apa inti hukum kekekalan massa?; the sheet answered from verified page context, surfacedMateri / Hukum Kekekalan Massa, and returned noWaduh/error state. Root-start logs for that request showverified: true,assetId/contentId,materialKey: lesson.chemistry.basic-chemistry-laws, and placementparentHref: /id/kurikulum/merdeka/kelas-10/kimia#hukum-dasar-kimia.Ciri-Ciri Reaksi Kimiacard underMateri Populer Minggu Iniand landed on/id/materi/kimia/hukum-dasar-kimia/ciri-ciri-reaksi-kimia./id/kurikulum/merdeka/kelas-10/kimiarendered class-10 Chemistry curriculum topics;/id/try-outrendered authenticated SNBT try-out sets;/id/latihan/snbt/pengetahuan-kuantitatif/tryout-2026/set-1/soal-1rendered the practice question page with choices and next/previous controls.Structure/DX audit
The latest follow-up adds no wrapper chains, no new facade/barrel, no alias shim, no generic utility dump, and no temporary migration/backfill surface.
Current direct evidence:
learningViewsnow has explicit ownership indexes for the actual read seams instead of relying on ambiguous row fallback.contents/views/impl.tsremains an Effect-native Convex implementation Module; private helpers split exact responsibilities: target resolution, latest device lookup, signed-in device lookup, safe existing-row selection, insert, update, recents, and signal enqueue.Touched latest-follow-up file sizes:
packages/backend/convex/contents/views/impl.ts: 349 LOC.packages/backend/convex/contents/schema.ts: 456 LOC.packages/backend/convex/contents/mutations/views.test.ts: 852 LOC. This test file was already a broad Convex mutation regression file and the user explicitly requested these identity regressions there; no production file exceeds the 500 LOC blocker in this follow-up.Effect/schema contract audit
The latest changed Convex seam stays Effect-native:
Effect.fnprograms with typedContentViewIoErrorfailures.Doc/MutationCtxand schema validators; the new indexes are defined in the Convex schema source of truth.learningViews.userId, but unsigned repeats no longer mutate a user-owned row, so the trigger cannot emit an event for a previous signed-in user from an unsigned repeat.Remaining TypeScript type/interface usage in this follow-up is limited to generated/library interop and local test helper shapes (
Doc,MutationCtx,ReturnType<typeof createConvexTestWithBetterAuth>, scheduled job arg inspection).Vercel applicability
No Vercel project configuration change is required for this follow-up. The PR changes Convex functions/schema plus tests;
React Doctor,react-doctor, andagent-docsare green on09d666ca7e. Final merge remains blocked only by required human review (reviewDecision=REVIEW_REQUIRED).