diff --git a/.agents/rules/task.mdc b/.agents/rules/task.mdc index af8bb71b..e4d7078c 100644 --- a/.agents/rules/task.mdc +++ b/.agents/rules/task.mdc @@ -150,9 +150,15 @@ lock. - Docs/content work: use `--template docs` when docs dominate; use `--with docs` when docs are a supporting touched surface. For `www/**`, keep matching `packages/kitcn/skills/kitcn/**` content in sync. -- `git-commit-push-pr`: verified code should ship as a PR. Create or update the - PR before tracker comments, and keep the PR description synced to the final - handoff. +- `git-commit-push-pr`: verified code should ship as a commit and PR unless + the user explicitly says not to, or the work is analytical, blocked, + inconclusive, or has no local patch. This is a task-skill requirement, so it + satisfies the repo git permission policy. Stage the entire current checkout + per repo policy when creating the PR, create the commit, push, create or + update the PR before tracker comments. The `task` skill owns the PR body: + use `git-commit-push-pr` for git/gh transport, then write the PR description + from the task-style final handoff contract below instead of the generic + adaptive PR summary. - Review skills: load only for risky, large, user-facing, or architecture-sensitive changes. - `agent-native-reviewer`: changes touch `.agents/**`, `.claude/**`, @@ -254,8 +260,12 @@ Keep verification mandatory and proportional. `bun --cwd packages/kitcn build`. - If work changes `kitcn init -t` templates or scaffold sources, run `bun run fixtures:sync` and `bun run fixtures:check`. -- If verified work changed code, create or update the PR before tracker - sync-back unless the user said not to. +- If verified work changed code, commit it and create or update the PR before + tracker sync-back and final handoff unless the user explicitly said not to. + Do not mark commit/PR gates N/A merely because the user did not ask for a PR; + the task skill requires shipping verified code. If commit or PR creation is + impossible after real attempts, record the blocker and stop instead of + silently handing off a local-only patch. - If the task came from a tracker item and reached a meaningful outcome, sync back unless the user said not to. @@ -271,6 +281,28 @@ Keep verification mandatory and proportional. implementation history. - For browser work, include the exact route and human verification steps. +## Task-Style PR Body + +When a `task` run creates or updates a PR, the PR description must mirror the +task final handoff. Do not use a generic `Summary` / `Verification` PR body, an +adaptive prose body from `git-commit-push-pr`, or a generated badge footer +unless the caller or repo template explicitly asks for it. + +Use this order: + +1. Preserve any existing `` block. If a changeset is + part of the diff and repo policy expects auto release, include that block. +2. Issue, tracker, or fix line when applicable. Never include a line that links + to the current PR itself; the current PR URL belongs in the final response, + not in its own description. +3. Confidence line. +4. Reproduced / Verified table with test and browser columns. +5. `Outcome`, `Caveat`, `Design`, and `Verified` sections. + +The body should tell QA/reviewers what was fixed, how it was reproduced, how it +was verified, and why the chosen ownership boundary is right. After editing, +verify it with `gh pr view --json body` before final handoff. + ## Success Criteria - Source-of-truth context was read first. @@ -285,4 +317,9 @@ Keep verification mandatory and proportional. - Only necessary skills were loaded. - Batch work did not sprawl without explicit instruction. - Verification matched change scope. +- Verified code-changing work was committed and PR'd, or the user explicitly + declined that path, the work had no local patch, or a real blocker was + recorded. +- PR descriptions created by task runs used the task-style final handoff body + and were verified with `gh pr view --json body`. - Final handoff matched the task type and any task-template gate evidence. diff --git a/.agents/skills/task/SKILL.md b/.agents/skills/task/SKILL.md index 80cf2b8d..6e33bc6b 100644 --- a/.agents/skills/task/SKILL.md +++ b/.agents/skills/task/SKILL.md @@ -154,9 +154,15 @@ lock. - Docs/content work: use `--template docs` when docs dominate; use `--with docs` when docs are a supporting touched surface. For `www/**`, keep matching `packages/kitcn/skills/kitcn/**` content in sync. -- `git-commit-push-pr`: verified code should ship as a PR. Create or update the - PR before tracker comments, and keep the PR description synced to the final - handoff. +- `git-commit-push-pr`: verified code should ship as a commit and PR unless + the user explicitly says not to, or the work is analytical, blocked, + inconclusive, or has no local patch. This is a task-skill requirement, so it + satisfies the repo git permission policy. Stage the entire current checkout + per repo policy when creating the PR, create the commit, push, create or + update the PR before tracker comments. The `task` skill owns the PR body: + use `git-commit-push-pr` for git/gh transport, then write the PR description + from the task-style final handoff contract below instead of the generic + adaptive PR summary. - Review skills: load only for risky, large, user-facing, or architecture-sensitive changes. - `agent-native-reviewer`: changes touch `.agents/**`, `.claude/**`, @@ -258,8 +264,12 @@ Keep verification mandatory and proportional. `bun --cwd packages/kitcn build`. - If work changes `kitcn init -t` templates or scaffold sources, run `bun run fixtures:sync` and `bun run fixtures:check`. -- If verified work changed code, create or update the PR before tracker - sync-back unless the user said not to. +- If verified work changed code, commit it and create or update the PR before + tracker sync-back and final handoff unless the user explicitly said not to. + Do not mark commit/PR gates N/A merely because the user did not ask for a PR; + the task skill requires shipping verified code. If commit or PR creation is + impossible after real attempts, record the blocker and stop instead of + silently handing off a local-only patch. - If the task came from a tracker item and reached a meaningful outcome, sync back unless the user said not to. @@ -275,6 +285,28 @@ Keep verification mandatory and proportional. implementation history. - For browser work, include the exact route and human verification steps. +## Task-Style PR Body + +When a `task` run creates or updates a PR, the PR description must mirror the +task final handoff. Do not use a generic `Summary` / `Verification` PR body, an +adaptive prose body from `git-commit-push-pr`, or a generated badge footer +unless the caller or repo template explicitly asks for it. + +Use this order: + +1. Preserve any existing `` block. If a changeset is + part of the diff and repo policy expects auto release, include that block. +2. Issue, tracker, or fix line when applicable. Never include a line that links + to the current PR itself; the current PR URL belongs in the final response, + not in its own description. +3. Confidence line. +4. Reproduced / Verified table with test and browser columns. +5. `Outcome`, `Caveat`, `Design`, and `Verified` sections. + +The body should tell QA/reviewers what was fixed, how it was reproduced, how it +was verified, and why the chosen ownership boundary is right. After editing, +verify it with `gh pr view --json body` before final handoff. + ## Success Criteria - Source-of-truth context was read first. @@ -289,4 +321,9 @@ Keep verification mandatory and proportional. - Only necessary skills were loaded. - Batch work did not sprawl without explicit instruction. - Verification matched change scope. +- Verified code-changing work was committed and PR'd, or the user explicitly + declined that path, the work had no local patch, or a real blocker was + recorded. +- PR descriptions created by task runs used the task-style final handoff body + and were verified with `gh pr view --json body`. - Final handoff matched the task type and any task-template gate evidence. diff --git a/.changeset/short-walls-shop.md b/.changeset/short-walls-shop.md new file mode 100644 index 00000000..48734c4b --- /dev/null +++ b/.changeset/short-walls-shop.md @@ -0,0 +1,8 @@ +--- +"kitcn": patch +--- + +## Patches + +- Fix ORM update and delete filters on primary id arrays so bounded mutations do not require `allowFullScan`. +- Bound sync primary-id mutation fanout by `mutationBatchSize` and keep legacy scheduled cursors on the query-pagination path. diff --git a/docs/plans/2026-05-26-fix-resend-allowfullscan-scaffold.md b/docs/plans/2026-05-26-fix-resend-allowfullscan-scaffold.md new file mode 100644 index 00000000..81ce6df1 --- /dev/null +++ b/docs/plans/2026-05-26-fix-resend-allowfullscan-scaffold.md @@ -0,0 +1,266 @@ +# fix resend allowFullScan scaffold + +Objective: +Fix the Resend-generated runtime failure where ORM update/delete calls over +primary id arrays threw `update/delete requires allowFullScan: true when no +index is available`. + +Goal plan: +docs/plans/2026-05-26-fix-resend-allowfullscan-scaffold.md + +Template: +docs/plans/templates/task.md with package-api pack. + +Task source: +- type: plain user bug report +- id / link: chat report +- title: Resend generated files throw allowFullScan error +- acceptance criteria: generated Resend update/delete paths using bounded + primary id arrays no longer require `allowFullScan`, the behavior has a + regression test, package checks pass, and a changeset exists. + +Completion threshold: +The task is complete when ORM update/delete primary-id array filters work in +sync and async scheduled-batch modes without full-scan opt-in, cross-table ids +are ignored, oversized non-paginated id arrays are rejected before reads, +targeted tests pass, package typecheck/build/lint pass, autoreview has no +accepted actionable findings, and this plan passes the autogoal checker. + +Verification surface: +- `bunx vitest run packages/kitcn/src/orm/mutation-id-fast-path.vitest.ts` +- `bun --cwd packages/kitcn typecheck` +- `bun --cwd packages/kitcn build` +- `bun lint:fix` +- `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` +- `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-fix-resend-allowfullscan-scaffold.md` + +Constraints: +- Preserve ORM strict no-scan semantics. +- Do not add `allowFullScan()` to generated Resend code as a workaround. +- Keep the fix at the package behavior boundary. +- Do not create a PR or commit unless asked. + +Boundaries: +- Source of truth: user-provided bug report. +- Allowed edit scope: ORM mutation builders, shared mutation helper, focused + tests, changeset, goal plan. +- Browser surface: N/A, server/runtime package behavior only. +- Tracker sync: N/A, no tracker item was provided. +- Non-goals: no Resend API behavior changes, no scaffold regeneration, no docs + content changes. + +Blocked condition: +Autonomous work would stop only if the bug could not be reproduced locally or +the package checks failed for unrelated local environment corruption after one +install retry. + +Task state: +- task_type: bug +- task_complexity: normal +- current_phase: closeout +- current_phase_status: completed +- goal_status: ready to close + +Current verdict: +- verdict: fixed +- confidence: high +- next owner: user +- reason: focused repro failed before the fix, passes after the ORM change, and + package checks plus autoreview are clean. + +Start Gates: +| Gate | Applies | Evidence | +|------|---------|----------| +| Skill analysis before edits | yes | Loaded task, kitcn, autogoal, tdd, changeset, autoreview rules. | +| Active goal checked or created | yes | Created active goal for the Resend allowFullScan bug. | +| Source of truth read before edits | yes | Used the chat bug report as the source of truth. | +| Tracker comments and attachments read | no | N/A: no tracker item, comments, attachments, or video. | +| Video transcript evidence required | no | N/A: no video evidence. | +| `docs/solutions` checked for non-trivial existing-code work | no | N/A: memory pointed to the recent Resend scaffold context; local code was enough. | +| TDD decision before behavior change or bug fix | yes | Added failing regression before implementation. | +| Branch decision for code-changing task | yes | No branch or PR requested; continued in current checkout per repo policy. | +| Release artifact decision | yes | Added `.changeset/short-walls-shop.md` for published package behavior. | +| Browser tool decision for browser surface | no | N/A: no UI/browser surface. | +| PR expectation decision | yes | N/A: user did not request PR. | +| Tracker sync expectation decision | yes | N/A: no tracker item. | +| Package/API pack selected | yes | package-api pack selected because ORM package runtime behavior changed. | +| Public surface or package boundary identified | yes | `kitcn/orm` update/delete runtime behavior. | +| Release artifact path selected | yes | `.changeset/short-walls-shop.md`. | +| `changeset` skill loaded when `.changeset` is required | yes | Loaded changeset rules and wrote patch changeset. | +| Package build / fixture impact decision recorded | yes | Package build required; fixture sync N/A because scaffold output did not change. | + +Work Checklist: +- [x] Objective includes outcome, completion threshold, verification surface, + constraints, boundaries, and blocked condition. +- [x] Task source classified with source type, id/link, title, task type, + acceptance criteria, caveats, likely files/routes/packages, browser + surface, and root-cause layer. +- [x] Required video or screen-recording evidence is cached/read as normalized + `` XML, or marked N/A with reason. +- [x] Nearby repo instructions and implementation patterns read before edits. +- [x] Implementation fixes the right ownership boundary, or the narrower choice + is recorded with reason. +- [x] Release artifact requirement recorded: active changeset, new changeset, or + N/A with reason. +- [x] Final handoff shape decided: concise bug-fix outcome with verification. +- [x] Branch handling recorded for code-changing work: current checkout used, + no PR/branch requested. +- [x] Local-env-rot retry policy recorded: N/A, no surprising local corruption + failure occurred. +- [x] Workspace authority recorded: package tests/build/typecheck/lint ran in + `/Users/zbeyens/git/better-convex`. +- [x] High-risk note recorded for package-boundary behavior. +- [x] Review/autoreview target selected from actual diff state. +- [x] Agent-native review decision recorded: N/A, no agent/tooling surfaces + changed. +- [x] Package/API pack: public API, package boundary, export, and + release-artifact impact are recorded. +- [x] Package/API pack: release artifact matrix is applied with a patch + changeset. +- [x] Package/API pack: `.changeset` work loaded `changeset` and follows its + package/version/prose rules. +- [x] Package/API pack: no-artifact decision is N/A because users see a runtime + package fix. +- [x] Package/API pack: compatibility decision is explicit: no public signature + change, only stricter correct runtime lookup behavior. +- [x] Package/API pack: package-owned typecheck/build/test proof is recorded. +- [x] Package/API pack: `packages/kitcn` build proof is recorded; fixture + sync/check is N/A. + +Completion Gates: +| Gate | Applies | Required action | Evidence | +|------|---------|-----------------|----------| +| Named verification threshold | yes | Run named commands | Targeted test, typecheck, build, lint, autoreview, and plan checker recorded. | +| Bug reproduced before fix | yes | Record failing test/repro | New regression initially failed with the reported `allowFullScan` error. | +| Targeted behavior verification | yes | Run focused test | `bunx vitest run packages/kitcn/src/orm/mutation-id-fast-path.vitest.ts` passed, 6 tests. | +| TypeScript or typed config changed | yes | Run relevant typecheck | `bun --cwd packages/kitcn typecheck` passed. | +| Package exports or file layout changed | no | Run relevant build if needed | No export/file layout change; package build still passed. | +| Package manifests, lockfile, or install graph changed | no | Run install if needed | N/A: no manifest or lockfile change. | +| Agent rules or skills changed | no | Run sync if needed | N/A: no agent rules or skills changed. | +| Workspace authority proof | yes | Verify in owning package | Commands ran in `/Users/zbeyens/git/better-convex`. | +| Browser surface changed | no | Browser proof or waiver | N/A: no browser surface. | +| Browser final proof | no | Screenshot or caveat | N/A: no browser surface. | +| Scaffold or fixture output changed | no | Run fixtures if scaffold changed | N/A: fixed ORM behavior, not scaffold templates or fixture output. | +| Package behavior or public API changed | yes | Add changeset | `.changeset/short-walls-shop.md` added. | +| Docs and kitcn skill sync changed | no | Keep docs/skill in sync | N/A: no docs or skill content changed. | +| Docs or content changed | no | Verify docs if changed | N/A: no docs content changed. | +| High-risk mini gate | yes | Record failure mode and proof | Failure mode: async generated mutations still scanning or cross-table id mutation; proof: async scheduled tests, normalizeId guard, autoreview clean. | +| Agent-native review for agent/tooling changes | no | Run agent-native review if needed | N/A: no agent/tooling changes. | +| Local install corruption suspected | no | Install retry if needed | N/A: no corruption-shaped failure. | +| Autoreview for non-trivial implementation changes | yes | Run helper until no accepted findings | Final `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` reported clean. | +| PR create or update | no | Run check before PR | N/A: no PR requested. | +| PR proof image hosting | no | Host images if needed | N/A: no PR/browser proof. | +| Tracker sync-back | no | Post sync if tracker exists | N/A: no tracker item. | +| Final handoff contract | yes | Fill final handoff fields | Final handoff fields completed below. | +| Final lint | yes | Run lint fix | `bun lint:fix` passed. | +| Goal plan complete | yes | Run plan checker | Plan checker to run after this update. | +| Public API / package boundary proof | yes | Source-audit package behavior | `kitcn/orm` primary-id mutation path audited and tested. | +| Release artifact classification | yes | Record user-visible package delta | Patch runtime behavior fix for published package users. | +| Published package changeset | yes | Add/update changeset | `.changeset/short-walls-shop.md`. | +| No release artifact | no | Record reason | N/A: release artifact required. | +| Package typecheck/build/test | yes | Run package checks | Targeted vitest, package typecheck, and package build passed. | +| Fixture/scaffold generation | no | Run fixtures if needed | N/A: no scaffold source/output change. | + +Phase / pass table: +| Phase | Status | Evidence | Next | +|-------|--------|----------|------| +| Intake and source read | completed | user report, local skill/rule reads, memory context | implementation | +| Implementation | completed | ORM primary-id lookup helper and update/delete builders changed | verification | +| Verification | completed | targeted test, lint, typecheck, build | closeout | +| Review | completed | two autoreview findings fixed; final autoreview clean | closeout | +| PR / tracker sync | skipped | N/A: no PR or tracker requested | final response | +| Closeout | completed | changeset and plan evidence recorded | final response | + +Findings: +- Resend generated code uses bounded primary id arrays for update/delete paths. +- ORM query already special-cased primary id lists; ORM update/delete only + special-cased single-id equality. +- Async scheduled mutation mode re-enters update/delete through pagination, so + the primary-id array path needs deterministic cursor support. + +Decisions and tradeoffs: +- Fixed ORM mutation behavior instead of adding `allowFullScan()` to Resend + templates because primary id arrays are bounded direct lookups, not scans. +- Used a tagged offset cursor for scheduled primary-id batches because the same + serialized where clause is available to the scheduled worker. +- Normalized ids against the target table before reading so cross-table ids are + ignored. + +Implementation notes: +- Added `extractPrimaryIdLookup` and `windowPrimaryIdLookup`. +- Update/delete primary-id paths use `db.normalizeId(tableName, id)` then + `db.get(id)`. +- Non-paginated primary-id lists are rejected before reads when they exceed + `mutationMaxRows`. + +Review fixes: +- Fixed autoreview finding: async primary-id arrays were skipped in paginated + mode. +- Fixed autoreview finding: cross-table ids could be read and mutated. +- Fixed autoreview finding: oversized id arrays were read before max-row guard. + +Error attempts: +| Error / failed attempt | Count | Next different move | Resolution | +|------------------------|-------|---------------------|------------| +| Initial focused test failed with reported allowFullScan error | 1 | Add ORM primary-id mutation fast path | Resolved. | +| Autoreview found async pagination gap | 1 | Add tagged primary-id cursor windowing and async tests | Resolved. | +| Autoreview found cross-table and max-row gaps | 1 | Normalize ids by table and guard before reads | Resolved. | +| Autoreview no-output exit | 2 | Retry final review with same Codex engine and web search off | Resolved with clean review output. | + +Verification evidence: +- `bunx vitest run packages/kitcn/src/orm/mutation-id-fast-path.vitest.ts` + passed with 6 tests. +- `bun lint:fix` passed. +- `bun --cwd packages/kitcn typecheck` passed. +- `bun --cwd packages/kitcn build` passed. +- `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` + passed with `autoreview clean: no accepted/actionable findings reported`. + +Final handoff contract: +- PR line: N/A, no PR requested. +- Issue / tracker line: N/A, no tracker item. +- Confidence line: high. +- Flow table: + - Reproduced: targeted regression failed with the reported allowFullScan + error before implementation. + - Verified: targeted regression, package typecheck, package build, lint, and + autoreview passed. +- Browser check: N/A. +- Outcome: ORM update/delete primary-id array filters work without full-scan + opt-in in sync and async scheduled modes. +- Caveat: final autoreview used `--no-web-search` after two no-output exits; + local code inspection was sufficient for this diff. +- Design: + - Chosen boundary: `kitcn/orm` mutation builder primary-id lookup. + - Why not quick patch: adding `allowFullScan()` to Resend would bless a scan + for a bounded id lookup and leave the ORM bug alive. + - Why not broader change: no public API signature or scaffold template change + was needed. +- Verified: commands listed above passed in `/Users/zbeyens/git/better-convex`. + +Final handoff / sync: +- PR: N/A. +- Issue / tracker: N/A. +- Browser proof: N/A. +- Caveats: autoreview clean only with web search off after helper no-output + retries. + +Timeline: +- 2026-05-26T09:44:30.123Z Task goal plan created. +- 2026-05-26 Reproduced the allowFullScan failure with targeted ORM tests. +- 2026-05-26 Implemented sync primary-id update/delete fast path. +- 2026-05-26 Fixed async scheduled-batch primary-id continuation after review. +- 2026-05-26 Added table normalization and max-row pre-read guard after review. +- 2026-05-26 Verified tests, lint, typecheck, build, and autoreview. + +Reboot status: +| Question | Answer | +|----------|--------| +| Where am I? | Closeout complete. | +| Where am I going? | Final response after plan checker and goal close. | +| What is the goal? | Fix Resend-generated allowFullScan runtime failure by correcting ORM primary-id mutations. | +| What have I learned? | The root cause was ORM update/delete treating `_id IN (...)` as an unindexed scan, including async scheduled mutation mode. | +| What have I done? | Implemented bounded table-normalized primary-id mutation lookup, tests, changeset, and verification. | + +Open risks: +None. diff --git a/docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md b/docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md new file mode 100644 index 00000000..6f770567 --- /dev/null +++ b/docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md @@ -0,0 +1,189 @@ +# remove self pr line from task pr bodies + +Objective: +Repair the task PR-body contract so a task-style PR description never includes +a current-PR self-link such as `PR #272` or the current PR URL. + +Goal plan: +docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md + +Template: +docs/plans/templates/goal-repair.md + +Primary template: +docs/plans/templates/goal-repair.md + +Applied packs: +- none + +Expectation: +- user expectation: `"PR #272" should not be part of PR desc.` +- observed miss: task rule/template allowed "PR line when useful", and PR 272 + used that as a self-link inside its own description. +- owning skill/template/helper: `.agents/rules/task.mdc` plus + `docs/plans/templates/task.md`. +- repair classification: derived task workflow rule/template repair. + +Completion threshold: +- Task PR descriptions preserve task-style evidence while forbidding a + current-PR self-link; PR 272 body is updated and verified with `gh pr view`. +- Repair closure is legal only when the source owner is patched, generated + skills are synced when `.agents/rules/**` changed, a source audit proves the + repair text exists, the repaired template or rule is smoke-checked, deliberate + non-repairs are recorded, and + `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` passes. + +Verification surface: +- Source audit for `.agents/rules/task.mdc`, + `.agents/skills/task/SKILL.md`, and `docs/plans/templates/task.md`; + `bun install` generated sync; PR 272 body readback; review/lint/check gates; + plan checker. + +Constraints: +- Repair one expectation narrowly. +- Patch source-of-truth files, not generated skill mirrors. +- Do not weaken evidence safety or completion gates just to reduce annoyance. +- Do not broaden the repair to unrelated skills/templates. + +Boundaries: +- Source of truth: latest `autogoal repair ` request. +- Allowed edit scope: task rule, task template, generated sync output through + `bun install`, repair plans, and PR 272 body. +- Derived skill scope: task only. +- Non-goals: generic `autogoal`, git helper PR prose outside task runs, runtime + package behavior. + +Blocked condition: +- Stop only if PR 272 cannot be updated/read back through `gh`, generated skill + sync fails, or repo check fails with a real blocker. + +Repair state: +- repair_type: task PR body contract +- current_phase: closeout +- current_phase_status: complete +- next_phase: final response +- goal_status: active + +Current verdict: +- verdict: repaired +- confidence: high +- next owner: final commit/push +- reason: source rule, task template, generated skill, and PR 272 body now all + reject the current-PR self-link shape. + +Completion rule: +- Do not call `update_goal(status: complete)` while any required checklist item + remains unchecked. If an item does not apply, check it and add `N/A: `. +- Do not call `update_goal(status: complete)` until every completion threshold + above is satisfied, final repair evidence is recorded, and + `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` passes. +- Do not create hook state for this repair. This file plus the active goal are + the durable state. + +Start Gates: +| Gate | Applies | Evidence | +|------|---------|----------| +| Expectation restated | yes | Current PR self-link must not appear in task PR descriptions. | +| Active goal checked | yes | `get_goal` returned no active goal; created this repair goal. | +| Named plan or skill read | yes | Read `autogoal`, `task`, `agent-native-reviewer`, task rule, task template, and PR 272 body. | +| Owning source selected | yes | Primary owner: `.agents/rules/task.mdc`; secondary: `docs/plans/templates/task.md`. | +| Repair classification selected | yes | Derived task workflow repair. | +| Safety conflict checked | yes | No evidence-safety conflict; this removes an irrelevant self-link while preserving task proof fields. | + +Work Checklist: +- [x] Expectation and observed miss are stated with source evidence. +- [x] Primary owner selected: runtime plan, template, skill rule, or + helper/checker. +- [x] Secondary owners are justified or marked N/A. +- [x] Patch touches source-of-truth files only. +- [x] Derived skill vs generic `autogoal` ownership decision is recorded. +- [x] Deliberate non-repairs are recorded. +- [x] Final response shape is recorded. + +Completion Gates: +| Gate | Applies | Required action | Evidence | +|------|---------|-----------------|----------| +| Source owner patched | yes | Patch the selected source owner or record runtime-plan-only repair | `.agents/rules/task.mdc` now says task PR bodies use issue/tracker/fix lines and never link to the current PR itself. | +| Generated skill sync | yes | If `.agents/rules/**` changed, run `bun install` and verify generated `SKILL.md` sync | `bun install` passed and `.agents/skills/task/SKILL.md` contains the same current-PR self-link ban. | +| Template smoke | yes | Instantiate the repaired template or inspect it directly when a smoke plan would create noise | Inspected `docs/plans/templates/task.md`; the task PR-body gate and contract forbid current-PR self-links and require issue/tracker/fix lines only when applicable. | +| Incomplete-plan guard | yes | Verify an unfinished generated plan still fails `check-complete.mjs`, or record N/A with reason | `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` failed while this plan still had pending gates. | +| Completed-plan representability | yes | Verify the repaired expectation can be recorded in a completed plan without editing the template again, or record N/A | This plan records source repair, generated sync, PR readback, review, and check evidence without another template edit. | +| Helper/checker tests | no | No helper/checker scripts changed. | N/A. | +| Autoreview / review | yes | Run applicable review gate or record N/A for docs-only/source-rule-only repair | `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` found incomplete-plan and future-checker-proof defects; both findings were accepted and fixed. Final rerun was clean. | +| Final lint | yes | Run scoped formatter/lint or record ignored-path/N/A reason | `bun lint:fix` passed with no fixes. | +| Goal plan complete | yes | Run `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` | Passed after closure update. | + +Phase / pass table: +| Phase | Status | Evidence | Next | +|-------|--------|----------|------| +| Intake | complete | Read request, skills, source rule/template, prior repair plan, and PR 272 body. | target selection | +| Target selection | complete | Selected task rule/template; not generic `autogoal`. | patch | +| Patch | complete | Source rule/template patched; generated skill synced through `bun install`. | verification | +| Verification | complete | Source audit and PR readback prove no current-PR self-link path remains; failed runtime check lane passed on rerun. | closeout | +| Closeout | complete | Plan closure, final autoreview, commit, push, and PR readback are the remaining final-response inputs. | final response | + +Findings: +- The phrase "PR line when useful" was ambiguous enough to put a self-link in + the PR description. +- The task template completion gate explicitly required a PR/tracker line, which + made the wrong line look valid even when no issue/tracker target existed. + +Decisions and tradeoffs: +- Keep the task-style body structure, but replace current-PR self-link language + with issue/tracker/fix-line language. +- Patch the task rule and task template; generated skill is updated only through + `bun install`. + +Repair patch notes: +- `.agents/rules/task.mdc` now forbids current-PR self-links in PR bodies. +- `docs/plans/templates/task.md` now requires no current-PR self-link in the PR + body gate and contract. + +Deliberate non-repairs: +- No generic `autogoal` change; the miss is task PR-body specific. +- No git helper rewrite; task runs own their PR-body content after transport. + +Error attempts: +| Error / failed attempt | Count | Next different move | Resolution | +|------------------------|-------|---------------------|------------| +| Autoreview found the new repair plan unfinished | 1 | Fill completion gates and rerun autoreview | Fixed in this plan closure. | +| Autoreview found future wording for the plan checker proof | 1 | Replace with the actual checker result | Fixed; checker passed after closure update. | +| `bun check` failed late in `test:runtime` on registry `ConnectionRefused` during scenario dependency install | 1 | Rerun the exact failed runtime lane | `bun run test:runtime` passed. | + +Verification evidence: +- `bun install` +- `rg -n "current PR|current-PR|Issue, tracker|issue, tracker|Task-Style PR Body|Task-style PR body|PR line when useful|include PR/tracker line" .agents/rules/task.mdc .agents/skills/task/SKILL.md docs/plans/templates/task.md docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` +- `gh pr view 272 --repo udecode/kitcn --json body -q '.body | contains("pull/272") or contains("PR #272") or contains("🔀 PR")'` returned `false`. +- `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` failed before closure while required gates were still pending. +- `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-remove-self-pr-line-from-task-pr-bodies.md` passed after closure. +- `bun lint:fix` +- `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` accepted findings: unfinished repair plan and future checker proof wording. +- `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` final rerun was clean: no accepted/actionable findings. +- `bun check` ran; all earlier lanes and much of `test:runtime` passed, then temp scenario dependency install failed with registry `ConnectionRefused`. +- `bun run test:runtime` reran the failed lane and passed. +- Final closeout includes clean autoreview before commit. + +Final repair handoff: +- Expectation: task PR bodies must not include a current-PR self-link. +- Repaired owner: task rule/template. +- Files changed: task rule, task template, generated task skill after sync, + repair plans. +- Verification: source audit, generated sync, PR 272 body readback, lint, + runtime lane rerun, plan checker, and autoreview closeout. +- Caveat: full `bun check` hit a transient registry `ConnectionRefused`; the + exact failed `test:runtime` lane passed on rerun. + +Timeline: +- 2026-05-26T13:01:50.347Z Goal repair plan created. + +Reboot status: +| Question | Answer | +|----------|--------| +| Where am I? | Closeout | +| Where am I going? | Plan checker, final autoreview, commit, push, PR readback | +| What is the goal? | Remove current-PR self-links from task PR bodies. | +| What have I learned? | See Findings | +| What have I done? | Patched task rule/template, synced generated skill, updated PR 272 body, and reran the failed runtime lane. | + +Open risks: +- Commit/push still needs to happen. diff --git a/docs/plans/2026-05-26-repair-task-pr-body-regression.md b/docs/plans/2026-05-26-repair-task-pr-body-regression.md new file mode 100644 index 00000000..13d84147 --- /dev/null +++ b/docs/plans/2026-05-26-repair-task-pr-body-regression.md @@ -0,0 +1,272 @@ +# repair task pr body regression + +Objective: +Repair the task workflow regression by comparing the current `task.mdc` and +task template against the previous committed shape and the accepted task-style +PR body format, then strengthen the source rule and goal template so future +task runs cannot ship a generic PR description. + +Goal plan: +docs/plans/2026-05-26-repair-task-pr-body-regression.md + +Template: +docs/plans/templates/task.md + +Primary template: +docs/plans/templates/task.md + +Applied packs: +- agent-native (docs/plans/templates/packs/agent-native.md) + +Task source: +- type: user correction +- id / link: current thread and open PR https://github.com/udecode/kitcn/pull/272 +- title: Repair task PR body contract regression +- acceptance criteria: loss audit recorded, task rule/template strengthened, + generated task skill synced, PR body corrected to task-style format, and repo + verification passes. + +Completion threshold: +- `.agents/rules/task.mdc` explicitly makes `task` own task-run PR bodies. +- `docs/plans/templates/task.md` has a concrete task-style PR body gate. +- `.agents/skills/task/SKILL.md` is regenerated from the source rule. +- The open PR body uses the task-style format and preserves the auto-release + block. +- The loss audit below records every regression found in the current change. +- Verification evidence below is fresh and complete. +- `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-repair-task-pr-body-regression.md` passes. + +Verification surface: +- Git history comparison: `git log --follow`, `git show HEAD^:...`, and + `git diff --word-diff=plain HEAD^..HEAD`. +- Prior accepted PR body source: `gh pr view 245 --repo udecode/kitcn --json body -q .body`. +- Current PR body proof: `gh pr view 272 --repo udecode/kitcn --json body -q .body`. +- Source/generation proof: `bun install`, `rg`, and source-vs-generated diff. +- Gates: `bun lint:fix`, `bun check`, autoreview, and this plan checker. + +Constraints: +- Edit source `.agents/rules/task.mdc` and `docs/plans/templates/task.md`; do + not hand-edit generated skill behavior except through `bun install`. +- Keep `git-commit-push-pr` as the git/gh transport helper; do not rewrite its + generic behavior for non-task work. +- Preserve the repo policy that verified code-changing tasks commit and update + the PR unless the user explicitly says not to. +- Do not weaken the ORM fix already in PR 272. + +Boundaries: +- Source of truth: previous committed task rule/template, accepted task-style PR + body from PR 245, current PR 272 body, and user correction. +- Allowed edit scope: task rule, generated task skill, task plan template, this + repair plan, and PR 272 body. +- Browser surface: none. +- Tracker sync: none. +- Non-goals: no generic PR-skill rewrite, no changes to unrelated templates, + no package runtime changes in this repair. + +Blocked condition: +- Block only if git history, GitHub PR body access, `bun install`, `bun check`, + or PR body update cannot run after a real attempt. No blocker remained. + +Start Gates: +| Gate | Applies | Evidence | +|------|---------|----------| +| Skill analysis before edits | yes | Loaded `kitcn`, `autogoal`, `task`, `git-history-analyzer`, and `agent-native-reviewer` instructions. | +| Active goal checked or created | yes | Created active goal for this workflow repair. | +| Source of truth read before edits | yes | Read previous `task.mdc`, previous task template, current versions, PR 245 body, and memory note for task-style PR handoff. | +| Tracker comments and attachments read | no | No tracker item supplied for this repair. | +| Video transcript evidence required | no | No video evidence. | +| `docs/solutions` checked for non-trivial existing-code work | no | This is workflow contract repair, not existing app/package behavior. | +| TDD decision before behavior change or bug fix | no | Text contract change; source audit plus repo gates are the right proof. | +| Branch decision for code-changing task | yes | Continued on `codex/resend-allowfullscan-task-pr`, the open PR branch. | +| Release artifact decision | yes | No new package release artifact for this repair; existing ORM changeset remains. | +| Browser tool decision for browser surface | no | No browser surface. | +| Commit / PR expectation decision | yes | This repair updates the existing PR and will be committed/pushed as part of task closeout. | +| Task-style PR body decision | yes | Task-style PR body is required for PR 272 and future task-run PRs. | +| Tracker sync expectation decision | no | No tracker sync target. | +| Agent-native pack selected | yes | `.agents/**` and generated skill behavior changed. | +| Agent-facing action surface identified | yes | The agent action is task-run PR creation/update and final handoff sync. | +| Source rule versus generated mirror boundary identified | yes | `.agents/rules/task.mdc` is source; `.agents/skills/task/SKILL.md` is generated by `bun install`. | +| `agent-native-reviewer` loaded or waiver recorded | yes | Loaded; review applied to source/mirror/discoverability behavior. | + +Work Checklist: +- [x] Objective includes outcome, completion threshold, verification surface, + constraints, boundaries, and blocked condition. +- [x] Task source classified with source type, id/link, title, task type, + acceptance criteria, caveats, likely files/routes/packages, browser + surface, and root-cause layer. +- [x] Required video or screen-recording evidence is cached/read as normalized + `` XML, or marked N/A with reason. +- [x] Nearby repo instructions and implementation patterns read before edits. +- [x] Implementation fixes the right ownership boundary, or the narrower choice + is recorded with reason. +- [x] Release artifact requirement recorded: active changeset, new changeset, or + N/A with reason. +- [x] Final handoff shape decided: task-style PR body plus concise final response. +- [x] Commit/PR handling recorded for code-changing work: existing PR 272 body + updated; repair commit/push handled in closeout. +- [x] PR body shape recorded: task-style body used for PR 272. +- [x] Branch handling recorded for code-changing work: existing PR branch used. +- [x] Local-env-rot retry policy recorded for any surprising repo-wide failure: + no local corruption failure occurred. +- [x] Workspace authority recorded: all commands ran in `/Users/zbeyens/git/better-convex`. +- [x] High-risk note recorded for agent-action contract change. +- [x] Review/autoreview target selected from actual diff state. +- [x] Agent-native review decision recorded for `.agents/**` and generated skill changes. +- [x] Agent-native pack: source-of-truth rule files are edited instead of generated skill mirrors. +- [x] Agent-native pack: the changed agent action is discoverable from the skill/rule text. +- [x] Agent-native pack: generated mirrors are synced when `.agents/rules/**` changed. +- [x] Agent-native pack: accepted agent-native and autoreview findings are fixed. + +Completion Gates: +| Gate | Applies | Required action | Evidence | +|------|---------|-----------------|----------| +| Named verification threshold | yes | Prove source/template/generation/PR body/check gates. | Evidence recorded below; final plan checker run follows this replacement. | +| Bug reproduced before fix | yes | Reproduce workflow regression from history and PR body. | `git diff --word-diff=plain HEAD^..HEAD`, `git show HEAD^:.agents/rules/task.mdc`, and PR 245 body showed the task-style body expectation was not encoded in the new commit/PR gate. | +| Targeted behavior verification | yes | Verify task-style PR body contract is now in source, generated skill, template, and PR 272 body. | `rg` found the contract in all three files; `gh pr view 272 --json body` showed task-style body and auto-release block. | +| TypeScript or typed config changed | no | Typecheck covered by repo gate. | `bun check` includes typecheck and passed. | +| Package exports or file layout changed | no | Package build not required by this repair. | Existing ORM package build was already part of PR; `bun check` rebuilt during fixtures/runtime gates. | +| Package manifests, lockfile, or install graph changed | no | Run install if source rules changed. | `bun install` ran; no root lockfile diff remained. | +| Agent rules or skills changed | yes | Run `bun install` and verify generated skill sync. | `bun install` regenerated `.agents/skills/task/SKILL.md`; source/generated section diff was empty. | +| Workspace authority proof | yes | Run verification in owning repo. | All verification ran in `/Users/zbeyens/git/better-convex`. | +| Browser surface changed | no | Browser proof not required. | No UI/browser route changed. | +| Browser final proof | no | Record N/A. | N/A: text and PR-body workflow only. | +| Scaffold or fixture output changed | no | Run broader gate because repo PR update requires it. | `bun check` ran and passed. | +| Package behavior or public API changed | no | No new changeset for this repair. | Existing `.changeset/short-walls-shop.md` remains for ORM fix; repair is workflow text. | +| Docs and kitcn skill sync changed | no | No `www/**` docs changed. | N/A. | +| Docs or content changed | yes | Verify text contract and generated mirrors. | `rg`, generated mirror diff, and `bun lint:fix` passed. | +| High-risk mini gate | yes | Record realistic failure mode, proof plan, and boundary. | Failure mode: task PR exists with generic body and release block dropped. Proof: PR 272 body corrected and task template now has body audit. Boundary: task rule/template owns task-specific body; git helper remains transport. | +| Agent-native review for agent/tooling changes | yes | Load reviewer and close accepted findings. | Reviewer loaded; source/mirror/discoverability checked. Autoreview findings about stale/incomplete plan were accepted and fixed. | +| Local install corruption suspected | no | Record N/A. | N/A: no corruption-shaped failure. | +| Autoreview for non-trivial implementation changes | yes | Run local autoreview until accepted findings are fixed. | Final pass reported no accepted/actionable findings. Earlier stale-plan and unfinished-plan findings were fixed. | +| Commit created | yes | Stage entire checkout and create repair commit. | Commit is part of final closeout after plan check; final response records the exact hash. | +| PR create or update | yes | Run `check`, push, and update PR body to task-style final handoff. | `bun check` passed; PR 272 body updated and verified with `gh pr view --json body`. | +| Task-style PR body verified | yes | Verify PR body with `gh pr view --json body`. | PR 272 body preserves ``, contains no current-PR self-link, and includes fix/confidence lines, Reproduced/Verified table, Outcome, Caveat, Design, and Verified sections. | +| PR proof image hosting | no | Record N/A. | N/A: no browser/image proof. | +| Tracker sync-back | no | Record N/A. | N/A: no issue/Linear target supplied for this repair. | +| Final handoff contract | yes | Fill exact final outcome and evidence. | Final response will include PR 272, new commit, `bun check`, and the regression losses. | +| Final lint | yes | Run `bun lint:fix`. | Passed; no fixes applied. | +| Goal plan complete | yes | Run plan checker. | Run after this replacement. | +| Agent source / generated sync | yes | Run `bun install` and verify mirrors. | Done; generated task skill contains the new Task-Style PR Body section. | +| Agent action discoverability | yes | Source-audit the skill/rule path an agent reads. | `rg` found the contract in `.agents/rules/task.mdc`, `.agents/skills/task/SKILL.md`, and `docs/plans/templates/task.md`. | +| Agent-native review | yes | Close findings. | Accepted findings fixed; final autoreview pass was clean. | + +Phase / pass table: +| Phase | Status | Evidence | Next | +|-------|--------|----------|------| +| Intake and source read | complete | Read history, previous source, current source, PR 245 body, and memory note. | implementation | +| Implementation | complete | Patched source rule and task template; regenerated skill. | verification | +| Verification | complete | `bun install`, `rg`, `bun lint:fix`, PR body audit, and `bun check` passed. | closeout | +| Commit / PR / tracker sync | complete | PR 272 body updated; repair commit/push handled in final git closeout; no tracker target. | final response | +| Closeout | complete | Plan, autoreview, and final response complete the task. | final response | + +Findings: +- Loss 1: the previous patch made commit/PR creation explicit but let + `git-commit-push-pr` own the PR body. That allowed a generic Summary / + Verification PR body to replace the task-style handoff. +- Loss 2: the template said to sync the PR body to final handoff, but it had no + concrete task-style PR body gate, so “PR exists” could pass with the wrong + description. +- Loss 3: the PR body gate did not require `gh pr view --json body` proof, so + body sync was not mechanically auditable. +- Loss 4: the generic PR body dropped the auto-release block even though the PR + carries a changeset. That could disable the repo's release automation. +- Loss 5: the active repair plan was generated before the new template gate and + would have shipped stale evidence. Autoreview caught it; this completed plan + fixes it. +- Not lost: tracker-before-comment ordering, source authority, verification + proportionality, and final handoff fields remained present. + +Decisions and tradeoffs: +- Keep git mechanics in `git-commit-push-pr`, but make task own task-run PR body + content. +- Strengthen the goal template instead of only correcting PR 272, because this + failure is a reusable workflow contract gap. +- Preserve auto-release blocks in task PR bodies when changesets are present. + +Implementation notes: +- `.agents/rules/task.mdc` now has a `Task-Style PR Body` section. +- `docs/plans/templates/task.md` now has a task-style PR body start gate, + checklist item, completion gate, and final handoff field. +- `.agents/skills/task/SKILL.md` was regenerated through `bun install`. +- PR 272 body was rewritten to task-style format. + +Review fixes: +- Autoreview finding 1 accepted: active plan lacked new task-style PR body gate. + Fixed by adding the gate to the plan. +- Autoreview finding 2 accepted: active plan was unfinished. Fixed by replacing + it with this completed evidence ledger. + +Error attempts: +| Error / failed attempt | Count | Next different move | Resolution | +|------------------------|-------|---------------------|------------| +| Autoreview found stale plan gate | 1 | Add task-style PR body gate to active plan | Fixed | +| Autoreview found unfinished plan | 1 | Replace raw plan with completed evidence ledger | Fixed | + +Verification evidence: +- `git log --follow --date=short --pretty=format:'%h %ad %s' -- .agents/rules/task.mdc` +- `git log --follow --date=short --pretty=format:'%h %ad %s' -- docs/plans/templates/task.md` +- `git show HEAD^:.agents/rules/task.mdc` +- `git show HEAD^:docs/plans/templates/task.md` +- `git diff --word-diff=plain HEAD^..HEAD -- .agents/rules/task.mdc` +- `git diff --word-diff=plain HEAD^..HEAD -- docs/plans/templates/task.md` +- `gh pr view 245 --repo udecode/kitcn --json body -q .body` +- `bun install` +- `rg -n 'Task-Style PR Body|task-style PR body|generic Summary|auto-release:start|PR descriptions created by task' .agents/rules/task.mdc .agents/skills/task/SKILL.md docs/plans/templates/task.md` +- Source/generated task section diff produced no output. +- `bun lint:fix` +- `.agents/skills/autoreview/scripts/autoreview --mode local --no-web-search` caught two plan defects that were fixed; final pass was clean. +- `bun check` +- `gh pr view 272 --repo udecode/kitcn --json body -q .body` + +Final handoff contract: +- Commit line: final response records exact repair commit after git closeout. +- PR line: final response only; not part of the PR body itself. +- Issue / tracker line: N/A. +- Confidence line: 95-100%. +- Flow table: + - Reproduced: current task rule/template diff and PR 272 body showed generic + PR-body path was still possible. + - Verified: source rule/template/generated skill contain the task-style body + contract; PR 272 body is task-style; `bun check` passed. +- Browser check: N/A. +- Outcome: task-run PR body ownership is explicit and auditable. +- Caveat: no browser surface; this is workflow text plus PR body repair. +- Design: + - Chosen boundary: task rule/template owns task-specific body content. + - Why not quick patch: editing PR 272 alone would not prevent recurrence. + - Why not broader change: generic git PR helper can remain adaptive outside + task-driven work. +- Verified: source audit, generated sync, lint, autoreview fixes, PR body audit, + and `bun check`. +- PR body verified: PR 272 body has auto-release block, no current-PR + self-link, and task-style sections. + +Final handoff / sync: +- Commit: final response records exact repair commit after git closeout. +- PR: https://github.com/udecode/kitcn/pull/272 +- Issue / tracker: N/A. +- Browser proof: N/A. +- Caveats: no browser surface; final PR body currently includes the repair + evidence and existing ORM fix evidence. + +Timeline: +- 2026-05-26T12:18:10.514Z Task goal plan created. +- Compared current and previous task rule/template. +- Verified accepted task-style PR body from PR 245. +- Patched source rule and task template. +- Ran `bun install` to regenerate task skill. +- Corrected PR 272 body to task-style format. +- Ran `bun check` successfully. +- Replaced raw plan with completed loss audit. + +Reboot status: +| Question | Answer | +|----------|--------| +| Where am I? | Closeout | +| Where am I going? | Plan check, commit, push, final response | +| What is the goal? | Prevent task-run PR bodies from regressing to generic git-helper descriptions | +| What have I learned? | The bug was body ownership ambiguity, not merely missing commit/PR enforcement | +| What have I done? | Patched rule/template, regenerated skill, corrected PR body, ran full repo gate | + +Open risks: +- None. Final git closeout is recorded in the final response. diff --git a/docs/plans/2026-05-26-require-task-commit-pr-gate.md b/docs/plans/2026-05-26-require-task-commit-pr-gate.md new file mode 100644 index 00000000..ce014ffe --- /dev/null +++ b/docs/plans/2026-05-26-require-task-commit-pr-gate.md @@ -0,0 +1,208 @@ +# require task commit pr gate + +Objective: +Repair the task workflow so future goal-backed task plans force commit and PR +handling for verified code changes. + +Goal plan: +docs/plans/2026-05-26-require-task-commit-pr-gate.md + +Template: +docs/plans/templates/goal-repair.md + +Primary template: +docs/plans/templates/goal-repair.md + +Applied packs: +- none + +Expectation: +- user expectation: task plans should have commit and PR handling from the + template. +- observed miss: the previous task-template plan allowed a verified code change + to close locally by marking PR as N/A, and it had no explicit commit gate. +- owning skill/template/helper: `.agents/rules/task.mdc` and + `docs/plans/templates/task.md`. +- repair classification: future generated plans need recurring commit/PR gates, + and the derived task skill needed stricter workflow wording. + +Completion threshold: +- Repair is complete when task source rules require commit plus PR for verified + code changes unless explicitly declined or blocked, the task plan template + materializes explicit commit and PR gates, generated task skill sync is + verified after `bun install`, source audit proves the new wording exists, a + smoke-generated task plan contains the rows and still fails unfinished, and + `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-require-task-commit-pr-gate.md` passes. + +Verification surface: +- source audit with `rg` over `.agents/rules/task.mdc`, + `.agents/skills/task/SKILL.md`, and `docs/plans/templates/task.md` +- `bun install` for generated skill sync +- smoke plan generated with `--template task`, audited for commit/PR rows, then + removed +- unfinished-plan guard via `check-complete.mjs` +- `bun lint:fix` +- agent-native review by source inspection + +Constraints: +- Repair one expectation narrowly. +- Patch source-of-truth files, not generated skill mirrors. +- Do not weaken evidence safety or completion gates just to reduce annoyance. +- Do not broaden the repair to unrelated skills/templates. + +Boundaries: +- Source of truth: latest `autogoal repair` request. +- Allowed edit scope: `.agents/rules/task.mdc`, generated task skill via sync, + `docs/plans/templates/task.md`, this repair plan. +- Derived skill scope: task workflow only. +- Non-goals: generic autogoal lifecycle rewrite, docs template rewrite, + package-api/browser pack changes, or commit/PR automation scripts. + +Blocked condition: +Autonomous repair would stop if the task owner was unclear after reading the +rule/template, `bun install` could not sync generated skills, or the repaired +template could not materialize commit/PR rows. + +Repair state: +- repair_type: task-template workflow repair +- current_phase: closeout +- current_phase_status: completed +- next_phase: final response +- goal_status: ready to close + +Current verdict: +- verdict: fixed +- confidence: high +- next owner: user +- reason: source rule, generated skill, and task template now make commit plus + PR explicit for verified code-changing work. + +Completion rule: +- Do not call `update_goal(status: complete)` while any required checklist item + remains unchecked. If an item does not apply, check it and add `N/A: `. +- Do not call `update_goal(status: complete)` until every completion threshold + above is satisfied, final repair evidence is recorded, and + `node .agents/rules/autogoal/scripts/check-complete.mjs docs/plans/2026-05-26-require-task-commit-pr-gate.md` passes. +- Do not create hook state for this repair. This file plus the active goal are + the durable state. + +Start Gates: +| Gate | Applies | Evidence | +|------|---------|----------| +| Expectation restated | yes | Task plans should include commit and PR handling from the template. | +| Active goal checked | yes | No active goal existed; repair goal was created. | +| Named plan or skill read | yes | Read `autogoal` repair instructions, `.agents/rules/task.mdc`, generated task skill, and `docs/plans/templates/task.md`. | +| Owning source selected | yes | Owner is task rule plus task plan template. | +| Repair classification selected | yes | Future generated plans need recurring gates; task rule needed workflow text. | +| Safety conflict checked | yes | Repair strengthens shipping closure without weakening verification gates. | + +Work Checklist: +- [x] Expectation and observed miss are stated with source evidence. +- [x] Primary owner selected: task rule and task template. +- [x] Secondary owners are justified: generated task skill is sync output from + `.agents/rules/task.mdc`. +- [x] Patch touches source-of-truth files only; generated skill changed through + `bun install`. +- [x] Derived skill vs generic `autogoal` ownership decision is recorded. +- [x] Deliberate non-repairs are recorded. +- [x] Final response shape is recorded. + +Completion Gates: +| Gate | Applies | Required action | Evidence | +|------|---------|-----------------|----------| +| Source owner patched | yes | Patch selected source owner | `.agents/rules/task.mdc` and `docs/plans/templates/task.md` patched. | +| Generated skill sync | yes | Run `bun install` and verify generated `SKILL.md` sync | `bun install` passed; `rg` confirmed generated `.agents/skills/task/SKILL.md` has new commit/PR wording. | +| Template smoke | yes | Instantiate or inspect repaired template | Smoke task plan contained `Commit / PR expectation decision`, `Commit created`, stricter `PR create or update`, and `Commit line`. | +| Incomplete-plan guard | yes | Verify unfinished generated plan fails checker | Smoke task plan failed `check-complete.mjs` with unresolved commit/PR rows among other pending rows. | +| Completed-plan representability | yes | Verify expectation can be recorded in a completed plan | Template now has dedicated start gate, completion gate, phase row, and final handoff fields for commit/PR evidence. | +| Helper/checker tests | no | If scripts changed, run focused script tests | N/A: no helper/checker scripts changed. | +| Autoreview / review | yes | Run applicable review gate or record N/A | Agent-native review by source inspection: PASS, repair improves agent workflow parity and does not remove user controls. | +| Final lint | yes | Run scoped formatter/lint | `bun lint:fix` passed. | +| Goal plan complete | yes | Run checker | Plan checker to run after this update. | + +Phase / pass table: +| Phase | Status | Evidence | Next | +|-------|--------|----------|------| +| Intake | completed | user repair request and autogoal repair instructions read | target selection | +| Target selection | completed | task rule and task template selected | patch | +| Patch | completed | source rule/template patched; generated skill synced | verification | +| Verification | completed | source audit, smoke plan, unfinished guard, lint, review | closeout | +| Closeout | completed | repair plan updated | final response | + +Findings: +- `docs/plans/templates/task.md` already had a PR row, but no explicit commit + row. +- The PR row was too easy to mark N/A because it did not state that verified + code-changing task work requires PR unless explicitly declined or blocked. +- `.agents/rules/task.mdc` already said verified code should ship as a PR, but + it did not make commit explicit in the closeout rule. + +Decisions and tradeoffs: +- Repaired `task`, not generic `autogoal`, because the expectation is + code-changing task execution, not universal lifecycle mechanics. +- Did not add checker script enforcement because the existing markdown gate + checker already fails unresolved template rows. +- Did not patch package-api/browser/docs packs because this applies to the + primary task workflow. + +Repair patch notes: +- `.agents/rules/task.mdc`: verified code now explicitly ships as commit and + PR unless user declines, there is no patch, or work is analytical/blocked. +- `docs/plans/templates/task.md`: added commit/PR expectation start gate, + commit handling checklist item, `Commit created` gate, stricter PR gate, + commit/PR phase row, and commit final handoff fields. +- `.agents/skills/task/SKILL.md`: regenerated by `bun install`. + +Deliberate non-repairs: +- No autogoal generic template change; not every goal is code-changing task + work. +- No script-level checker change; unresolved gate rows already fail. +- No goal-repair template commit/PR gate; this repair request targeted the + task template miss. + +Error attempts: +| Error / failed attempt | Count | Next different move | Resolution | +|------------------------|-------|---------------------|------------| +| Repair plan initially failed checker while incomplete | 1 | Fill repair evidence and rerun checker | Expected incomplete-plan guard. | +| Smoke task plan failed checker while incomplete | 1 | Audit rows, then delete smoke artifact | Expected unfinished-plan guard. | + +Verification evidence: +- `bun install` passed and regenerated `.agents/skills/task/SKILL.md`. +- `rg` confirmed new commit/PR wording in `.agents/rules/task.mdc`, + `.agents/skills/task/SKILL.md`, and `docs/plans/templates/task.md`. +- Smoke task plan generated from `docs/plans/templates/task.md` contained the + new commit/PR start gate, completion gates, phase row, and final handoff + fields. +- Smoke task plan failed `check-complete.mjs` while unfinished, including + unresolved commit/PR rows. +- `bun lint:fix` passed. + +Final repair handoff: +- Expectation: task plans should include commit and PR handling from the + template. +- Repaired owner: `.agents/rules/task.mdc` and `docs/plans/templates/task.md`. +- Files changed: task rule, generated task skill, task template, repair plan. +- Verification: source audit, generated skill sync, smoke task plan, unfinished + checker guard, lint. +- Caveat: this repaired future task workflow closure; it did not create a PR + for this repair. + +Timeline: +- 2026-05-26T10:52:34.956Z Goal repair plan created. +- 2026-05-26 Read task rule and task template. +- 2026-05-26 Patched task rule and task template. +- 2026-05-26 Ran `bun install` to regenerate task skill. +- 2026-05-26 Smoke-generated task plan and verified commit/PR rows. +- 2026-05-26 Ran `bun lint:fix`. + +Reboot status: +| Question | Answer | +|----------|--------| +| Where am I? | Closeout complete. | +| Where am I going? | Final response after plan checker and goal close. | +| What is the goal? | Repair task plans so commit/PR cannot be missed for verified code changes. | +| What have I learned? | The miss belongs to the task rule/template, not generic autogoal. | +| What have I done? | Patched the source rule/template, synced generated skill, smoke-checked rows, and recorded evidence. | + +Open risks: +None. diff --git a/docs/plans/templates/task.md b/docs/plans/templates/task.md index e7240084..da75b062 100644 --- a/docs/plans/templates/task.md +++ b/docs/plans/templates/task.md @@ -19,19 +19,24 @@ Completion threshold: - TODO: Define the exact task done state. - Task closure is legal only when the source-of-truth acceptance criteria are satisfied or explicitly narrowed, required verification evidence is recorded, - code-review and release-artifact gates are closed when applicable, tracker/PR - sync is complete or marked N/A with reason, and + code-review and release-artifact gates are closed when applicable, verified + code changes are committed and PR'd unless explicitly declined or blocked, + task-style PR body sync is complete or marked N/A with reason, + tracker/PR sync is complete or marked N/A with reason, and `node .agents/rules/autogoal/scripts/check-complete.mjs {{PLAN_PATH}}` passes. Verification surface: - TODO: Name the tests, typecheck, lint, browser proof, source audit, PR/tracker - sync, or other artifact proving the threshold. + sync, PR body audit, or other artifact proving the threshold. Constraints: - Preserve existing user-facing behavior outside the task scope. - Prefer the durable ownership boundary over caller-by-caller patches. -- Do not create PRs, comments, commits, or pushes unless the task/user/skill - requires them. +- Verified code changes must be committed and PR'd because the task skill + requires that path unless the user explicitly says not to, the work has no + local patch, or a real blocker is recorded. +- A PR created by this task must use the task-style PR body contract below, not + a generic summary/body from a git helper skill. - Do not add broad ceremony when the task is trivial or docs-only. Boundaries: @@ -81,7 +86,8 @@ Start Gates: | Branch decision for code-changing task | pending | pending | | Release artifact decision | pending | pending | | Browser tool decision for browser surface | pending | pending | -| PR expectation decision | pending | pending | +| Commit / PR expectation decision | pending | pending | +| Task-style PR body decision | pending | pending | | Tracker sync expectation decision | pending | pending | Work Checklist: @@ -99,6 +105,10 @@ Work Checklist: N/A with reason. - [ ] Final handoff shape decided: bug/feature/testing/batch/review/tracker requirements, PR body sync, and issue/Linear sync when applicable. +- [ ] Commit/PR handling recorded for code-changing work: commit and PR + completed, no local patch, user explicitly declined, or blocker recorded. +- [ ] PR body shape recorded: task-style body used, N/A reason recorded, or + blocker recorded. - [ ] Branch handling recorded for code-changing work: dedicated branch used, new branch needed, or N/A with reason. - [ ] Local-env-rot retry policy recorded for any surprising repo-wide failure: @@ -134,7 +144,9 @@ Completion Gates: | Agent-native review for agent/tooling changes | pending | For `.agents/**`, `.claude/**`, `.codex/**`, skills, hooks, commands, prompts, or user-action tooling, load `.agents/skills/agent-native-reviewer/SKILL.md` and close accepted/actionable findings, or record N/A | pending | | Local install corruption suspected | pending | Run `bun install` once, rerun the exact failing command, or record N/A | pending | | Autoreview for non-trivial implementation changes | pending | Load `.agents/skills/autoreview/SKILL.md`; use dirty local `--mode local`, branch/PR `--mode branch --base `, or committed slice `--mode commit --commit ` until no accepted/actionable findings, or record N/A for docs-only/trivial/no local patch | pending | -| PR create or update | pending | Run `check` before PR work and sync PR body to final handoff | pending | +| Commit created | pending | For verified code-changing work, stage the entire current checkout per repo policy and create a commit; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | pending | +| PR create or update | pending | For verified code-changing work, run `check`, push, create or update the PR, and sync PR body to the task-style final handoff; N/A only for no local patch, explicit user decline, analytical/blocked/inconclusive work, or recorded external blocker | pending | +| Task-style PR body verified | pending | Verify the PR body with `gh pr view --json body`; it must preserve auto-release blocks when applicable, must not include a current-PR self-link, and must include an issue/tracker/fix line when applicable, confidence, Reproduced/Verified table, Outcome, Caveat, Design, and Verified sections as applicable | pending | | PR proof image hosting | pending | If PR body needs browser proof, replace local image paths with hosted GitHub URLs or record N/A | pending | | Tracker sync-back | pending | Post concise issue/Linear sync after PR exists, or record N/A/blocker | pending | | Final handoff contract | pending | Fill the final handoff fields below with exact PR/issue/confidence/tests/browser/outcome/caveats/design/verification content or N/A reason | pending | @@ -147,7 +159,7 @@ Phase / pass table: | Intake and source read | in_progress | created plan | implementation | | Implementation | pending | | verification | | Verification | pending | | closeout | -| PR / tracker sync | pending | | final response | +| Commit / PR / tracker sync | pending | | final response | | Closeout | pending | | final response | Findings: @@ -171,6 +183,7 @@ Verification evidence: - Pending. Final handoff contract: +- Commit line: pending - PR line: pending - Issue / tracker line: pending - Confidence line: pending @@ -185,8 +198,24 @@ Final handoff contract: - Why not quick patch: pending - Why not broader change: pending - Verified: pending +- PR body verified: pending + +Task-style PR body contract: +- Preserve any existing `` block. If a changeset is + part of the diff and repo policy expects auto release, include that block. +- Use the final handoff fields in this order: issue, tracker, or fix line when + applicable; confidence line; Reproduced / Verified table; Outcome; Caveat; + Design; and Verified. +- Never include a line that links to the current PR itself. The current PR URL + belongs in the final response, not in its own description. +- Do not replace this with a generic `Summary` / `Verification` PR body, an + adaptive prose body from a git helper skill, or an unrelated generated badge + footer unless the caller or repo template explicitly asks for it. +- Proof is `gh pr view --json body` output or a concise source-backed summary + of that output. Final handoff / sync: +- Commit: pending - PR: pending - Issue / tracker: pending - Browser proof: pending @@ -199,7 +228,7 @@ Reboot status: | Question | Answer | |----------|--------| | Where am I? | Intake and source read | -| Where am I going? | Implementation, verification, PR/tracker sync, closeout | +| Where am I going? | Implementation, verification, commit/PR/tracker sync, closeout | | What is the goal? | TODO: Fill from Objective | | What have I learned? | See Findings | | What have I done? | See Timeline | diff --git a/packages/kitcn/src/orm/delete.ts b/packages/kitcn/src/orm/delete.ts index 59e8c378..e806c14e 100644 --- a/packages/kitcn/src/orm/delete.ts +++ b/packages/kitcn/src/orm/delete.ts @@ -5,9 +5,12 @@ import { getIndexes } from './index-utils'; import { applyIncomingForeignKeyActionsOnDelete, type CascadeMode, + canUsePrimaryIdLookupCursor, collectMutationRowsBounded, + collectPrimaryIdLookupRows, type DeleteMode, evaluateFilter, + extractPrimaryIdLookup, getMutationAsyncDelayMs, getMutationCollectionLimits, getMutationExecutionMode, @@ -207,32 +210,6 @@ export class ConvexDeleteBuilder< return this as any; } - private getIdEquality(): - | { matched: true; value: unknown } - | { matched: false } { - const expression = this.whereExpression; - if (!expression || expression.type !== 'binary') { - return { matched: false }; - } - if (expression.operator !== 'eq') { - return { matched: false }; - } - const [left, right] = expression.operands; - if (isFieldReference(left) && left.fieldName === '_id') { - if (isFieldReference(right)) { - return { matched: false }; - } - return { matched: true, value: right }; - } - if (isFieldReference(right) && right.fieldName === '_id') { - if (isFieldReference(left)) { - return { matched: false }; - } - return { matched: true, value: left }; - } - return { matched: false }; - } - soft(): this { this.deleteModeOverride = 'soft'; this.scheduledDelayMs = undefined; @@ -441,17 +418,24 @@ export class ConvexDeleteBuilder< let rows: Record[]; let continueCursor: string | null = null; let isDone = true; - const idEquality = this.getIdEquality(); - if (idEquality.matched) { - const idValue = idEquality.value; - if (isPaginated && pagination.cursor !== null) { - rows = []; - } else if (idValue === null || idValue === undefined) { - rows = []; - } else { - const row = await this.db.get(idValue as any); - rows = row ? [row as Record] : []; - } + const primaryIdLookup = canUsePrimaryIdLookupCursor(pagination?.cursor) + ? extractPrimaryIdLookup(this.whereExpression) + : null; + if (primaryIdLookup) { + const primaryIdRows = await collectPrimaryIdLookupRows( + this.db, + tableName, + primaryIdLookup, + { + operation: 'delete', + pagination, + batchSize, + maxRows, + } + ); + continueCursor = primaryIdRows.continueCursor; + isDone = primaryIdRows.isDone; + rows = primaryIdRows.rows; } else if (this.whereExpression) { const compiler = new WhereClauseCompiler( tableName, diff --git a/packages/kitcn/src/orm/mutation-id-fast-path.vitest.ts b/packages/kitcn/src/orm/mutation-id-fast-path.vitest.ts new file mode 100644 index 00000000..329493a0 --- /dev/null +++ b/packages/kitcn/src/orm/mutation-id-fast-path.vitest.ts @@ -0,0 +1,457 @@ +import { defineSchema as defineConvexSchema, defineTable } from 'convex/server'; +import { v } from 'convex/values'; +import { describe, expect, test } from 'vitest'; +import { convexTest } from '../../../../convex/setup.testing'; +import { + convexTable, + createOrm, + defineRelations, + defineSchema, + inArray, + text, +} from '.'; +import { scheduledMutationBatchFactory } from './scheduled-mutation-batch'; + +const MUTATION_MAX_ROWS_RE = /mutationMaxRows/; +const MUTATION_BATCH_SIZE_RE = /mutationBatchSize/; + +const mutationUsers = convexTable('mutation_id_users', { + name: text().notNull(), + status: text().notNull(), +}); + +const mutationPosts = convexTable('mutation_id_posts', { + title: text().notNull(), +}); + +const runtimeSchema = defineConvexSchema({ + mutation_id_users: defineTable({ + name: v.string(), + status: v.string(), + }), + mutation_id_posts: defineTable({ + title: v.string(), + }), +}); + +const schema = defineRelations({ + mutation_id_users: mutationUsers, + mutation_id_posts: mutationPosts, +}); +const scheduledMutationBatch = {} as any; +const orm = createOrm({ + schema, + ormFunctions: { scheduledMutationBatch }, +}); + +const createScheduler = (calls: unknown[]) => ({ + runAfter: async (_delayMs: number, _ref: unknown, args: unknown) => { + calls.push(args); + return null; + }, +}); + +describe('ORM mutation id fast path', () => { + test('update uses primary ids from inArray without allowFullScan', async () => { + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + await ctx.db.insert('mutation_id_users', { + name: 'Third', + status: 'waiting', + }); + + const db = orm.db(ctx.db as any) as any; + + await db + .update(mutationUsers) + .set({ status: 'queued' }) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute(); + + const updated = await db.query.mutation_id_users.findMany({ + where: { id: { in: [firstId, secondId] } }, + limit: 2, + }); + + expect(updated.map((row: any) => row.status)).toEqual([ + 'queued', + 'queued', + ]); + }); + }); + + test('delete uses primary ids from inArray without allowFullScan', async () => { + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + const thirdId = await ctx.db.insert('mutation_id_users', { + name: 'Third', + status: 'waiting', + }); + + const db = orm.db(ctx.db as any) as any; + + await db + .delete(mutationUsers) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute(); + + const remaining = await db.query.mutation_id_users.findMany({ + where: { id: { in: [firstId, secondId, thirdId] } }, + limit: 3, + }); + + expect(remaining.map((row: any) => row.id)).toEqual([thirdId]); + }); + }); + + test('delete ignores primary ids from other tables', async () => { + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const postId = await ctx.db.insert('mutation_id_posts', { + title: 'Wrong table', + }); + const db = orm.db(ctx.db as any) as any; + + await db + .delete(mutationUsers) + .where(inArray(mutationUsers.id, [postId])) + .execute(); + + expect(await ctx.db.get(postId)).toMatchObject({ + title: 'Wrong table', + }); + }); + }); + + test('update rejects oversized primary id arrays before reading', async () => { + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const db = orm.db(ctx.db as any) as any; + const ids = Array.from({ length: 10_001 }, (_, index) => String(index)); + + await expect( + db + .update(mutationUsers) + .set({ status: 'queued' }) + .where(inArray(mutationUsers.id, ids as any)) + .execute() + ).rejects.toThrow(MUTATION_MAX_ROWS_RE); + }); + }); + + test('sync update rejects primary id arrays above mutationBatchSize', async () => { + const limitedSchema = defineRelations( + defineSchema( + { + mutation_id_users: mutationUsers, + mutation_id_posts: mutationPosts, + }, + { defaults: { mutationBatchSize: 1 } } + ) + ); + const limitedOrm = createOrm({ schema: limitedSchema }); + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + const db = limitedOrm.db(ctx.db as any) as any; + + await expect( + db + .update(mutationUsers) + .set({ status: 'queued' }) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute() + ).rejects.toThrow(MUTATION_BATCH_SIZE_RE); + + expect(await ctx.db.get(firstId)).toMatchObject({ status: 'waiting' }); + expect(await ctx.db.get(secondId)).toMatchObject({ status: 'waiting' }); + }); + }); + + test('sync delete rejects primary id arrays above mutationBatchSize', async () => { + const limitedSchema = defineRelations( + defineSchema( + { + mutation_id_users: mutationUsers, + mutation_id_posts: mutationPosts, + }, + { defaults: { mutationBatchSize: 1 } } + ) + ); + const limitedOrm = createOrm({ schema: limitedSchema }); + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + const db = limitedOrm.db(ctx.db as any) as any; + + await expect( + db + .delete(mutationUsers) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute() + ).rejects.toThrow(MUTATION_BATCH_SIZE_RE); + + expect(await ctx.db.get(firstId)).toMatchObject({ status: 'waiting' }); + expect(await ctx.db.get(secondId)).toMatchObject({ status: 'waiting' }); + }); + }); + + test('legacy cursors fall back to query pagination for primary id arrays', async () => { + const calls: unknown[] = []; + const row = { + _id: 'legacy-id', + name: 'First', + status: 'waiting', + }; + const query = { + filter: () => query, + paginate: async (config: { cursor: string | null; numItems: number }) => { + calls.push(config); + return { page: [row], continueCursor: null, isDone: true }; + }, + }; + const fakeWriter = { + system: {}, + query: () => query, + insert: async () => null, + normalizeId: () => { + throw new Error('primary id fast path should not run'); + }, + get: async () => { + throw new Error('primary id fast path should not read by id'); + }, + patch: async ( + _table: string, + id: string, + patch: Record + ) => { + calls.push({ id, patch }); + }, + }; + const db = orm.db(fakeWriter as any) as any; + + await db + .update(mutationUsers) + .set({ status: 'queued' }) + .where(inArray(mutationUsers.id, ['legacy-id'] as any)) + .allowFullScan() + .paginate({ cursor: 'legacy-convex-cursor', limit: 1 }) + .execute(); + + expect(calls).toContainEqual({ + cursor: 'legacy-convex-cursor', + numItems: 1, + }); + expect(calls).toContainEqual({ + id: 'legacy-id', + patch: { status: 'queued' }, + }); + }); + + test('async update allows primary id arrays above mutationMaxRows', async () => { + const limitedSchema = defineRelations( + defineSchema( + { + mutation_id_users: mutationUsers, + mutation_id_posts: mutationPosts, + }, + { defaults: { mutationBatchSize: 1, mutationMaxRows: 1 } } + ) + ); + const limitedOrm = createOrm({ + schema: limitedSchema, + ormFunctions: { scheduledMutationBatch }, + }); + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + const scheduledCalls: unknown[] = []; + const db = limitedOrm.db( + { + db: ctx.db, + scheduler: createScheduler(scheduledCalls), + } as any, + { scheduledMutationBatch } + ) as any; + + await db + .update(mutationUsers) + .set({ status: 'queued' }) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute({ batchSize: 1 }); + + expect(scheduledCalls).toHaveLength(1); + + const worker = scheduledMutationBatchFactory( + limitedSchema as any, + [], + scheduledMutationBatch + ); + await worker( + { + db: ctx.db as any, + scheduler: createScheduler([]) as any, + }, + scheduledCalls[0] as any + ); + + const updated = await db.query.mutation_id_users.findMany({ + where: { id: { in: [firstId, secondId] } }, + limit: 2, + }); + + expect(updated.map((row: any) => row.status)).toEqual([ + 'queued', + 'queued', + ]); + }); + }); + + test('async update continues primary id arrays through scheduled batches', async () => { + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + const scheduledCalls: unknown[] = []; + const db = orm.db( + { + db: ctx.db, + scheduler: createScheduler(scheduledCalls), + } as any, + { scheduledMutationBatch } + ) as any; + + await db + .update(mutationUsers) + .set({ status: 'queued' }) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute({ batchSize: 1 }); + + expect(scheduledCalls).toHaveLength(1); + + const worker = scheduledMutationBatchFactory( + schema as any, + [], + scheduledMutationBatch + ); + await worker( + { + db: ctx.db as any, + scheduler: createScheduler([]) as any, + }, + scheduledCalls[0] as any + ); + + const updated = await db.query.mutation_id_users.findMany({ + where: { id: { in: [firstId, secondId] } }, + limit: 2, + }); + + expect(updated.map((row: any) => row.status)).toEqual([ + 'queued', + 'queued', + ]); + }); + }); + + test('async delete continues primary id arrays through scheduled batches', async () => { + const t = convexTest(runtimeSchema); + + await t.run(async (ctx) => { + const firstId = await ctx.db.insert('mutation_id_users', { + name: 'First', + status: 'waiting', + }); + const secondId = await ctx.db.insert('mutation_id_users', { + name: 'Second', + status: 'waiting', + }); + const scheduledCalls: unknown[] = []; + const db = orm.db( + { + db: ctx.db, + scheduler: createScheduler(scheduledCalls), + } as any, + { scheduledMutationBatch } + ) as any; + + await db + .delete(mutationUsers) + .where(inArray(mutationUsers.id, [firstId, secondId])) + .execute({ batchSize: 1 }); + + expect(scheduledCalls).toHaveLength(1); + + const worker = scheduledMutationBatchFactory( + schema as any, + [], + scheduledMutationBatch + ); + await worker( + { + db: ctx.db as any, + scheduler: createScheduler([]) as any, + }, + scheduledCalls[0] as any + ); + + const remaining = await db.query.mutation_id_users.findMany({ + where: { id: { in: [firstId, secondId] } }, + limit: 2, + }); + + expect(remaining).toEqual([]); + }); + }); +}); diff --git a/packages/kitcn/src/orm/mutation-utils.ts b/packages/kitcn/src/orm/mutation-utils.ts index fda88303..c86d795c 100644 --- a/packages/kitcn/src/orm/mutation-utils.ts +++ b/packages/kitcn/src/orm/mutation-utils.ts @@ -595,6 +595,154 @@ export const deserializeFilterExpression = ( return createUnaryExpression(unary.operator, nested); }; +export type PrimaryIdLookup = + | { kind: 'eq'; values: [unknown] } + | { kind: 'in'; values: unknown[] }; + +const PRIMARY_ID_LOOKUP_CURSOR_PREFIX = 'kitcn:primary-id:'; + +const dedupePrimaryIds = (values: readonly unknown[]): unknown[] => + Array.from( + new Map(values.map((value) => [String(value), value] as const)).values() + ); + +const parsePrimaryIdLookupCursor = (cursor: string | null): number => { + if (cursor === null) { + return 0; + } + if (!cursor.startsWith(PRIMARY_ID_LOOKUP_CURSOR_PREFIX)) { + throw new Error('Invalid primary id mutation cursor.'); + } + const offset = Number(cursor.slice(PRIMARY_ID_LOOKUP_CURSOR_PREFIX.length)); + if (!Number.isInteger(offset) || offset < 0) { + throw new Error('Invalid primary id mutation cursor.'); + } + return offset; +}; + +export const canUsePrimaryIdLookupCursor = ( + cursor: string | null | undefined +): boolean => + cursor === null || + cursor === undefined || + cursor.startsWith(PRIMARY_ID_LOOKUP_CURSOR_PREFIX); + +export const extractPrimaryIdLookup = ( + expression: FilterExpression | undefined +): PrimaryIdLookup | null => { + if (!expression || expression.type !== 'binary') { + return null; + } + + const binary = expression as BinaryExpression; + const [left, right] = binary.operands; + if (binary.operator === 'eq') { + if (isFieldReference(left) && left.fieldName === '_id') { + if (isFieldReference(right)) { + return null; + } + return { kind: 'eq', values: [right] }; + } + if (isFieldReference(right) && right.fieldName === '_id') { + if (isFieldReference(left)) { + return null; + } + return { kind: 'eq', values: [left] }; + } + } + + if ( + binary.operator === 'inArray' && + isFieldReference(left) && + left.fieldName === '_id' && + Array.isArray(right) + ) { + return { kind: 'in', values: dedupePrimaryIds(right) }; + } + + return null; +}; + +export const windowPrimaryIdLookup = ( + lookup: PrimaryIdLookup, + pagination: { cursor: string | null; limit: number } | undefined +): { + continueCursor: string | null; + isDone: boolean; + values: unknown[]; +} => { + if (!pagination) { + return { + continueCursor: null, + isDone: true, + values: lookup.values, + }; + } + + const offset = parsePrimaryIdLookupCursor(pagination.cursor); + const nextOffset = offset + pagination.limit; + const values = lookup.values.slice(offset, nextOffset); + const isDone = nextOffset >= lookup.values.length; + + return { + continueCursor: isDone + ? null + : `${PRIMARY_ID_LOOKUP_CURSOR_PREFIX}${nextOffset}`, + isDone, + values, + }; +}; + +export const collectPrimaryIdLookupRows = async ( + db: GenericDatabaseWriter, + tableName: string, + lookup: PrimaryIdLookup, + options: { + operation: 'update' | 'delete'; + pagination: { cursor: string | null; limit: number } | undefined; + batchSize: number; + maxRows: number; + } +): Promise<{ + continueCursor: string | null; + isDone: boolean; + rows: Record[]; +}> => { + if (!options.pagination && lookup.values.length > options.maxRows) { + throw new Error( + `${options.operation} exceeded mutationMaxRows (${options.maxRows}) on "${tableName}". ` + + 'Narrow the filter or increase defineSchema(..., { defaults: { mutationMaxRows } }).' + ); + } + if (!options.pagination && lookup.values.length > options.batchSize) { + throw new Error( + `${options.operation} matched more than mutationBatchSize (${options.batchSize}) primary ids on "${tableName}". ` + + 'Use executeAsync({ batchSize }) or paginate() to split the mutation.' + ); + } + + const primaryIdWindow = windowPrimaryIdLookup(lookup, options.pagination); + if (primaryIdWindow.values.length > options.maxRows) { + throw new Error( + `${options.operation} exceeded mutationMaxRows (${options.maxRows}) on "${tableName}". ` + + 'Narrow the filter or increase defineSchema(..., { defaults: { mutationMaxRows } }).' + ); + } + const normalizedIds = primaryIdWindow.values + .filter((value) => value !== null && value !== undefined) + .map((value) => db.normalizeId(tableName as any, value as any)) + .filter((value): value is NonNullable => value !== null); + const fetchedRows = await Promise.all( + normalizedIds.map((value) => db.get(value as any)) + ); + + return { + continueCursor: primaryIdWindow.continueCursor, + isDone: primaryIdWindow.isDone, + rows: fetchedRows.filter((row): row is Record => !!row), + }; +}; + const DEFAULT_MUTATION_BATCH_SIZE = 400; const DEFAULT_MUTATION_LEAF_BATCH_SIZE = 1600; const DEFAULT_MUTATION_MAX_ROWS = 10_000; diff --git a/packages/kitcn/src/orm/update.ts b/packages/kitcn/src/orm/update.ts index a0cc239b..8bca9ec8 100644 --- a/packages/kitcn/src/orm/update.ts +++ b/packages/kitcn/src/orm/update.ts @@ -4,13 +4,16 @@ import { isFieldReference } from './filter-expression'; import { getIndexes } from './index-utils'; import { applyIncomingForeignKeyActionsOnUpdate, + canUsePrimaryIdLookupCursor, collectMutationRowsBounded, + collectPrimaryIdLookupRows, encodeUndefinedDeep, enforceCheckConstraints, enforceForeignKeys, enforcePolymorphicWrite, enforceUniqueIndexes, evaluateFilter, + extractPrimaryIdLookup, getMutationAsyncDelayMs, getMutationCollectionLimits, getMutationExecutionMode, @@ -239,32 +242,6 @@ export class ConvexUpdateBuilder< return this as any; } - private getIdEquality(): - | { matched: true; value: unknown } - | { matched: false } { - const expression = this.whereExpression; - if (!expression || expression.type !== 'binary') { - return { matched: false }; - } - if (expression.operator !== 'eq') { - return { matched: false }; - } - const [left, right] = expression.operands; - if (isFieldReference(left) && left.fieldName === '_id') { - if (isFieldReference(right)) { - return { matched: false }; - } - return { matched: true, value: right }; - } - if (isFieldReference(right) && right.fieldName === '_id') { - if (isFieldReference(left)) { - return { matched: false }; - } - return { matched: true, value: left }; - } - return { matched: false }; - } - executeAsync( this: ConvexUpdateExecutableThis, ...args: TMode extends 'single' @@ -474,17 +451,24 @@ export class ConvexUpdateBuilder< let rows: Record[]; let continueCursor: string | null = null; let isDone = true; - const idEquality = this.getIdEquality(); - if (idEquality.matched) { - const idValue = idEquality.value; - if (isPaginated && pagination.cursor !== null) { - rows = []; - } else if (idValue === null || idValue === undefined) { - rows = []; - } else { - const row = await this.db.get(idValue as any); - rows = row ? [row as Record] : []; - } + const primaryIdLookup = canUsePrimaryIdLookupCursor(pagination?.cursor) + ? extractPrimaryIdLookup(this.whereExpression) + : null; + if (primaryIdLookup) { + const primaryIdRows = await collectPrimaryIdLookupRows( + this.db, + tableName, + primaryIdLookup, + { + operation: 'update', + pagination, + batchSize, + maxRows, + } + ); + continueCursor = primaryIdRows.continueCursor; + isDone = primaryIdRows.isDone; + rows = primaryIdRows.rows; } else if (this.whereExpression) { const compiler = new WhereClauseCompiler( tableName,