Skip to content

Round 2 audit: lower-severity correctness, contract and tooling items #349

Description

@cavidelizade

Summary

A tracking list of the lower-severity and needs-a-closer-look items from the round 2 audit. Splitting each into its own issue felt like noise, so they're grouped here. Happy to break any of these out if you'd rather track them separately.

Backend

  • Cycle end date is treated as exclusive: a date-only end of today marks the cycle "completed" from midnight, so it reads completed on its own last day (service/cycle.go).
  • Per-project sequence IDs can be reused after a soft-delete (NextSequenceID uses MAX(...) WHERE deleted_at IS NULL), and there's no UNIQUE(project_id, sequence_id) backstop. Cosmetic today because reads filter deleted rows, but the invariant rests on one call site.
  • Full-row Save on AddIssueToEpic, RemoveIssueFromEpic, CreateEpic and RestoreDescriptionVersion can revert a concurrent field update. The main Update path avoids this with a changed-columns map; these didn't adopt it.
  • CompleteCycle snapshots and transfers in two separate transactions, so a failed transfer leaves the cycle "completed" with its work not moved, and a retry re-snapshots different numbers.
  • Importer can get stuck in "processing" forever if the process crashes mid-run (redelivery correctly skips processing jobs, but nothing recovers a crashed one).
  • AddAssignee doesn't validate the assignee is a workspace member, unlike the validateRelations path used by create/update.
  • Google OAuth treats an absent verified_email as verified; GitHub and GitLab fail closed here.
  • RabbitMQ consumer shares one channel across per-queue goroutines and republishes retries on it; amqp091 channels aren't safe for concurrent use.
  • applyPRSideEffects moves an issue to in-progress on PR reopen even if it's already Done, contradicting its own comment (github_events.go).
  • Sync target states (inProgressStateID/doneStateID) aren't validated to belong to the project.

Frontend

  • DescriptionEditor external-content sync can overwrite unsaved local edits if the parent pushes a changed initialHtml mid-edit.
  • Reaction toggle in IssueReactions/CommentReactions does set-state after await with no unmount guard.
  • FavoritesContext value isn't memoized (every other context here is), causing consumer churn; favorites are also fetched twice into the same context.
  • Spreadsheet column-resize grip can also start a native column drag and reorder; its window listeners are only removed on pointerup, not on unmount.
  • Axios 401 interceptor fires the AUTH_UNAUTHORIZED event on the logged-out startup me probe.

Contract / config / tooling

  • Phantom TS fields the backend never emits: PageApiResponse.title (model uses name), IssueViewApiResponse.owned_by.
  • Go fields missing from TS: cycle progress_snapshot/timezone/version, view is_locked/rich_filters; notification sender can be 'system', which the TS union omits.
  • types/index.ts Cycle.status union omits draft/current (so CyclesPage's status === 'started' branch is dead); Module.status uses in-progress while the canonical id is in_progress.
  • issue_view.access integer meaning is inverted vs page.access (0/1 swapped); a shared helper would silently flip. Views also can't set access through the typed create path.
  • tsconfig.app.json doesn't enable noUncheckedIndexedAccess, which given the heavy array indexing would catch real undefined bugs.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions