Skip to content

fix(settings): cap review-nag cooldown#2634

Merged
JSONbored merged 3 commits into
mainfrom
codex/fix-unbounded-review-nag-cooldown-vulnerability
Jul 3, 2026
Merged

fix(settings): cap review-nag cooldown#2634
JSONbored merged 3 commits into
mainfrom
codex/fix-unbounded-review-nag-cooldown-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent a repo-controlled reviewNagCooldownDays from overflowing Date arithmetic and throwing RangeError during issue_comment webhook processing by bounding the operational lookback window.
  • Keep existing behavior and defaults while making manifest, DB, OpenAPI, and runtime usage resilient to malicious or accidental oversized values.

Description

  • Add a public constant MAX_REVIEW_NAG_COOLDOWN_DAYS = 365 in src/settings/agent-actions.ts and use it as the system-wide cap.
  • Reject oversized manifest values at parse time with a warning in src/signals/focus-manifest.ts so .gittensory.yml entries above the cap are ignored.
  • Introduce normalizeReviewNagCooldownDays in src/db/repositories.ts to clamp persisted and DB-resolved values to the cap while preserving other normalization semantics.
  • Clamp the resolved cooldown at runtime before computing the sinceIso Date in src/queue/processors.ts so issue_comment handling cannot throw even if an oversized value reaches the processor.
  • Update src/openapi/schemas.ts and .gittensory.yml.example to document/enforce the 365-day maximum and regenerate the UI OpenAPI artifact (apps/gittensory-ui/public/openapi.json).
  • Add regression tests covering manifest parsing rejection, DB normalization (including legacy raw DB value), and the webhook path to ensure a huge configured window does not crash processing (tests updated in test/unit/focus-manifest.test.ts, test/unit/data-spine.test.ts, and test/unit/queue.test.ts).

Testing

  • Ran npm run typecheck and it succeeded.
  • Regenerated OpenAPI with npm run ui:openapi and validated with npm run ui:openapi:check, both succeeding and apps/gittensory-ui/public/openapi.json updated.
  • Ran focused unit tests: npx vitest run test/unit/focus-manifest.test.ts -t "review-nag cooldown settings", npx vitest run test/unit/data-spine.test.ts -t "persists and reads", and npx vitest run test/unit/queue.test.ts -t "oversized review-nag cooldown", and the new/affected tests passed.
  • Note: a full run of the large queue.test.ts file in this container hit existing long-running sweep timeouts unrelated to this fix, so a targeted test invocation was used to validate the regression; npm audit --audit-level=moderate could not complete here due to the registry audit endpoint returning 403 Forbidden.

Codex Task

@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 01:05:05 UTC

10 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change consistently bounds `reviewNagCooldownDays` at the manifest, repository read/write, OpenAPI, and processor layers, which addresses the Date overflow path without changing the default behavior. The runtime guard in `maybeThrottleReviewNagPing` is deliberately redundant with repository normalization, and the added tests cover both real normalization paths and the defensive processor clamp. I do not see a reachable correctness break in the visible diff.

Nits — 6 non-blocking
  • nit: `src/signals/focus-manifest.ts:744` duplicates `MAX_REVIEW_NAG_COOLDOWN_DAYS` while other changed modules import it, so future cap changes can drift unless this moves to a lightweight shared settings constants module.
  • nit: `test/unit/queue.test.ts:11979` uses a direct `resolveRepositorySettings` mock to exercise a state normal code paths now prevent; keep it, but the test name/comment should make clear this is defense-in-depth coverage rather than the primary regression path.
  • Move `MAX_REVIEW_NAG_COOLDOWN_DAYS` into a dependency-light module such as `src/settings/review-nag.ts`, then import it from `focus-manifest`, `repositories`, `openapi/schemas`, `processors`, and `agent-actions`.
  • Consider exporting and reusing the cooldown normalization helper outside `src/db/repositories.ts` so `src/queue/processors.ts:8818` can clamp through the same positive-integer semantics instead of only applying `Math.min`.
  • Add one manifest boundary assertion for exactly `365` in `test/unit/focus-manifest.test.ts` so the accepted upper bound is pinned alongside the `366` rejection.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 61 registered-repo PR(s), 52 merged, 506 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 61 PR(s), 506 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
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: 61 PR(s), 506 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 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

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.09%. Comparing base (6ff28c4) to head (aa061ba).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2634   +/-   ##
=======================================
  Coverage   96.09%   96.09%           
=======================================
  Files         237      237           
  Lines       26472    26478    +6     
  Branches     9602     9603    +1     
=======================================
+ Hits        25438    25444    +6     
  Misses        425      425           
  Partials      609      609           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.58% <100.00%> (+<0.01%) ⬆️
src/openapi/schemas.ts 100.00% <ø> (ø)
src/queue/processors.ts 92.39% <ø> (ø)
src/settings/agent-actions.ts 94.28% <100.00%> (+0.04%) ⬆️
src/signals/focus-manifest.ts 99.28% <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.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 2, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui aa061ba Commit Preview URL

Branch Preview URL
Jul 03 2026, 12:51 AM

JSONbored added 3 commits July 2, 2026 17:47
… import

focus-manifest.ts is part of the UI package's typechecked closure (its
transitive deps get walked by apps/gittensory-ui's own tsc run). Importing
MAX_REVIEW_NAG_COOLDOWN_DAYS from settings/agent-actions.ts pulled that
module's own import of github/commands.ts -> utils/crypto.ts into the UI
build for the first time, exposing a pre-existing latent
Uint8Array<ArrayBufferLike>/BufferSource type mismatch in crypto.ts that
the UI's tsc had never previously reached.

Duplicates the small constant locally in focus-manifest.ts instead of
importing it, keeping db/repositories.ts's own import from agent-actions.ts
(never part of the UI closure) unaffected. Also rebases the branch's merge
commit into a linear rebase onto current main.
…y exercise the guard

Gate review: the original regression test seeded an oversized
reviewNagCooldownDays through upsertRepositorySettings, but both
upsertRepositorySettings and getRepositorySettings already clamp that field
on write AND read -- so the value read back inside resolveRepositorySettings
was never actually oversized by the time maybeThrottleReviewNagPing saw it.
The test could pass even with processors.ts's own
Math.min(reviewNagCooldownDays, MAX_REVIEW_NAG_COOLDOWN_DAYS) guard removed
entirely.

Mocks resolveRepositorySettings directly (bypassing the DB/yml clamp layers
entirely, not just the write-time one) to actually deliver an oversized
value to the function under test, then proves the guard's effect
behaviorally: three prior pings 400 days old fall outside the CORRECTLY
capped 365-day window (no cooldown applied), whereas an uncapped
"1-billion-day" window would count them and trip the threshold. Confirmed
by mutation-testing: removing the Math.min throws a real RangeError
(Invalid time value) building the Date.
@JSONbored
JSONbored force-pushed the codex/fix-unbounded-review-nag-cooldown-vulnerability branch from d2d484e to aa061ba Compare July 3, 2026 00:49
@dosubot dosubot Bot added the size:M label Jul 3, 2026
@JSONbored
JSONbored merged commit d4aea8f into main Jul 3, 2026
14 checks passed
@JSONbored
JSONbored deleted the codex/fix-unbounded-review-nag-cooldown-vulnerability branch July 3, 2026 01:15
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.

Development

Successfully merging this pull request may close these issues.

1 participant