Skip to content

fix(api): preserve owner-close ai-review setting - #1583

Merged
JSONbored merged 1 commit into
mainfrom
codex/propose-fix-for-ai-review-close-owner-bug
Jun 27, 2026
Merged

fix(api): preserve owner-close ai-review setting#1583
JSONbored merged 1 commit into
mainfrom
codex/propose-fix-for-ai-review-close-owner-bug

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The AI-review update route accidentally reset the per-repo closeOwnerAuthors flag to false when callers omitted the new field because the Zod schema defaulted it to false and the handler unconditionally persisted that value.

Description

  • Make the AI-review payload accept closeOwnerAuthors as optional instead of defaulting to false by changing the schema to closeOwnerAuthors: z.boolean().optional() in src/api/routes.ts.
  • Preserve an existing closeOwnerAuthors value when the maintainer AI-review update omits the field by persisting parsed.data.closeOwnerAuthors ?? current.closeOwnerAuthors in src/api/routes.ts.
  • Add a regression unit test preserves closeOwnerAuthors when an AI-review update omits it to test/unit/routes-ai-byok.test.ts that seeds closeOwnerAuthors: true, issues an AI-review PUT without the field, and verifies the value remains true.

Testing

  • Ran git diff --check and npm run typecheck, both succeeded locally with no new type errors.
  • Ran the modified unit test file with npx vitest run test/unit/routes-ai-byok.test.ts and all tests in that file passed (20/20).
  • Ran npm run ui:openapi which regenerated apps/gittensory-ui/public/openapi.json successfully.
  • Attempted npm run test:coverage but coverage remapping failed after tests passed due to an upstream dependency mismatch (js-tokens@4.0.0 vs ast-v8-to-istanbul needing js-tokens@^10.0.0) producing TypeError: jsTokens is not a function, and npm audit --audit-level=moderate could not complete due to a registry 403; the functional unit tests and typecheck are green.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.46%. Comparing base (00cff73) to head (1d4ebcb).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1583   +/-   ##
=======================================
  Coverage   95.46%   95.46%           
=======================================
  Files         202      202           
  Lines       21839    21840    +1     
  Branches     7885     7886    +1     
=======================================
+ Hits        20849    20850    +1     
  Misses        414      414           
  Partials      576      576           
Files with missing lines Coverage Δ
src/api/routes.ts 94.59% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 31bdd3a into main Jun 27, 2026
18 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-ai-review-close-owner-bug branch June 27, 2026 08:35
@loopover-orb

loopover-orb Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review — safe to merge

2 files · 1 AI reviewers · no blockers · readiness 48/100 · CI green · unknown

✅ Approved — safe to merge

Review summary
Changes `closeOwnerAuthors` from `.default(false)` to `.optional()` and uses `??` to fall back to `current.closeOwnerAuthors` when the field is omitted — a correct, minimal fix for the accidental-reset bug. The `??` operator correctly distinguishes `undefined` (omitted) from an explicit `false`, so a caller that deliberately disables the flag is still honored. The regression test seeds the flag to `true`, issues a real PUT without the field, and asserts both the response and the DB row — this is a genuine integration test on the production code path, not a fabricated scenario.

Signal Result Evidence
Code review ✅ No blockers 1 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/20 Readiness component derived from cached public PR metadata and labels; size label size:XS.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 26 open PR(s), 9 likely reviewable, 17 unlinked.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 80 PR(s), 312 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 2 non-blocking
  • Consistency gap: `allAuthors` at `src/api/routes.ts:717` still uses `.default(false)`, and the UI's `saveConfig` in `ai-review-settings.tsx` does not send `allAuthors` either (it only posts `{ mode, byok, provider, model }`), so every UI save resets `allAuthors` to `false` — the same root bug this PR fixes for `closeOwnerAuthors`. Apply the same treatment: `allAuthors: z.boolean().optional()` + `aiReviewAllAuthors: parsed.data.allAuthors ?? current.aiReviewAllAuthors` (verify the `current` field name) and extend the test to cover this arm.
  • Apply `z.boolean().optional()` + `?? current.allAuthors` (check the exact `current` property name) to `allAuthors` at `src/api/routes.ts:717` and `:2255`, mirroring this PR's fix — the UI omits it on every save, making the existing default silently destructive for any repo that has set it to `true` via the API.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 80 PR(s), 312 issue(s).
  • Related work: Titles/paths share 5 meaningful terms. (PR #1582)
  • Related work: Titles/paths share 4 meaningful terms. (PR #1586)
  • Related work: Titles/paths share 5 meaningful terms. (PR #1581, PR #1587)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Review top overlaps.
  • Add scope summary.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

Changes `closeOwnerAuthors` from `.default(false)` to `.optional()` and uses `??` to fall back to `current.closeOwnerAuthors` when the field is omitted — a correct, minimal fix for the accidental-reset bug. The `??` operator correctly distinguishes `undefined` (omitted) from an explicit `false`, so a caller that deliberately disables the flag is still honored. The regression test seeds the flag to `true`, issues a real PUT without the field, and asserts both the response and the DB row — this is a genuine integration test on the production code path, not a fabricated scenario.

Nits (2)

  • Consistency gap: `allAuthors` at `src/api/routes.ts:717` still uses `.default(false)`, and the UI's `saveConfig` in `ai-review-settings.tsx` does not send `allAuthors` either (it only posts `{ mode, byok, provider, model }`), so every UI save resets `allAuthors` to `false` — the same root bug this PR fixes for `closeOwnerAuthors`. Apply the same treatment: `allAuthors: z.boolean().optional()` + `aiReviewAllAuthors: parsed.data.allAuthors ?? current.aiReviewAllAuthors` (verify the `current` field name) and extend the test to cover this arm.
  • Apply `z.boolean().optional()` + `?? current.allAuthors` (check the exact `current` property name) to `allAuthors` at `src/api/routes.ts:717` and `:2255`, mirroring this PR's fix — the UI omits it on every save, making the existing default silently destructive for any repo that has set it to `true` via the API.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant