fix(comprehension): finalize Story 3-3 — manual QA + cross-cutting fixes

Bundle of Story 3-3 follow-up patches and three regressions surfaced
during the live manual QA session on 2026-04-11.

## Manual QA fixes (cross-cutting)

* Epic 1 regression — get_installations_for_user called
  GET /user/installations, which 403s for OAuth-App tokens (only
  GitHub-App user-auth tokens work there). Refactored to verify access
  locally for User-type installs and via /user/orgs for Org-type
  installs. Added read:org to OAUTH_SCOPES. Updated test_router mock.
  This regression silently broke GET /api/v1/sessions/{id} (502 Bad
  Gateway) and would have shipped to production if Story 3-3 hadn't
  exercised the path end-to-end.

* Story 3-3 SSE auth wiring (3 compounding bugs):
  - useSSE.ts doc claimed JWT lived in a cookie; it lives in the
    Zustand store. EventSource cannot send custom headers.
  - ChatPanel built a relative SSE URL; Vite dev does not proxy to
    the API on port 8000.
  - get_current_user only read the Authorization header.
  Fix: backend accepts ?access_token= query-param fallback (security
  trade-off documented inline). Frontend builds an absolute URL with
  the JWT in the query string. API_BASE exported from
  shared/api/client.ts. 2 new dependency tests for the new auth path.

* Story 3-2 dark-mode contrast — react-diff-view's default CSS uses
  pale-green / pale-red backgrounds with `color: initial`, making the
  diff text unreadable on the dark surface. Override the --diff-* CSS
  custom properties in index.css. Cascade order fix: import the library
  CSS in main.tsx BEFORE index.css.

## Story 3-3 follow-up patches

Bundled together: SSE generator polish, repository test additions,
agents test, diff_refs test, store/useSSE test additions, alembic
migration tweaks. See individual file diffs.

## QA results

20/26 checklist items checked, 6 explicitly skipped per Option 3
scoping (rationale documented in the story file's Manual QA Results
section). 4 deferred follow-ups documented (no SSE replay on resume,
StrictMode double-mount rollback, misleading Connection-lost banner
for 4xx, react-diff-view markdown tokenize warning).

Story 3-3 transitions awaiting-manual-qa → done in sprint-status.yaml.

## Verification

259 backend tests + 57 frontend tests + ruff + ruff format + eslint
all green on the bundled state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: mariuspruvot

.gitignore

@@ -22,6 +22,10 @@ apps/web/dist/
 .env.local
 .env.*.local
 
+# Local docker compose overrides (may contain secrets like GitHub App private keys for local dev)
+docker-compose.override.yml
+docker-compose.override.yaml
+
 # IDE
 .idea/
 .vscode/

_bmad-output/implementation-artifacts/sprint-status.yaml

@@ -45,7 +45,7 @@
 #   'awaiting-manual-qa', unless invoked with `--skip-manual-qa-gate <reason>`.
 
 generated: 2026-04-09
-last_updated: 2026-04-11 # 3-3 → review (dev-story complete: question generation + SSE streaming)
+last_updated: 2026-04-11 # 3-3 → done after live manual QA session (Project Lead Marius). 20/26 checklist items checked, 6/26 explicitly skipped with rationale (Option 3 scoping). Session surfaced + fixed: 1 Epic 1 regression (get_installations_for_user / OAuth scopes), 1 Story 3-3 SSE auth wiring (3 compounding bugs), 1 Story 3-2 dark-mode contrast. 4 deferred follow-ups filed. 259 backend + 57 frontend tests green after fixes.
 project: helprs
 project_key: NOKEY
 tracking_system: file-system
@@ -114,7 +114,21 @@ development_status:
   # replaced "Story 3.3 backlog" prose with grep-able TODO(story-3.3).
   # make lint && vitest = clean + 24 web tests green (24 = 25 −3 +2).
   3-2-split-view-session-ui-and-diff-viewer: done
-  3-3-question-generation-and-sse-streaming: review
+  # Manual QA completed 2026-04-11 by Project Lead. 20/26 items checked,
+  # 6 explicitly skipped per Option 3 scoping (rationale documented in
+  # the story file's "Manual QA Results" section). QA session surfaced
+  # AND fixed 3 bugs: (1) Epic 1 regression — get_installations_for_user
+  # called /user/installations which 403s for OAuth-App tokens; refactored
+  # to use local DB + /user/orgs and added read:org scope; (2) Story 3-3
+  # SSE auth wiring — useSSE doc claimed cookie auth but JWT was in
+  # memory; ChatPanel built relative URL; get_current_user only read
+  # header; fixed via query-param fallback + absolute URL. (3) Story 3-2
+  # dark-mode CSS — react-diff-view default theme unreadable on dark
+  # surface; overrode --diff-* CSS vars + reordered import cascade.
+  # 4 deferred follow-ups filed (no SSE replay on resume, StrictMode
+  # double-mount rollback, misleading "Connection lost" for 4xx,
+  # markdown tokenize warning). NONE of these block 3-3 done.
+  3-3-question-generation-and-sse-streaming: done
   3-4-answer-submission-and-feedback-with-code-links: backlog
   3-5-role-adaptation-beyond-diff-and-large-pr-handling: backlog
   epic-3-retrospective: optional

apps/api/alembic/versions/a1b2c3d4e5f6_add_questions_table_and_total_.py

@@ -25,6 +25,11 @@
 
 def upgrade() -> None:
     # ---- sessions.total_questions (additive, nullable=False + default) ----
+    # The server_default is used ONLY to backfill existing rows during the
+    # ADD COLUMN. We drop it immediately afterward (P13 from story-3.3
+    # review) so the application layer is the only source of truth for
+    # this value — forgetting to set ``total_questions`` in an INSERT
+    # should fail loud, not silently land at 0.
     op.add_column(
         "sessions",
         sa.Column(
@@ -34,8 +39,14 @@ def upgrade() -> None:
             server_default=sa.text("0"),
         ),
     )
+    op.alter_column("sessions", "total_questions", server_default=None)
 
     # ---- questions table -------------------------------------------------
+    # Note (P14): no separate ``ix_questions_session_id`` index is
+    # created — the UniqueConstraint on ``(session_id, number)`` already
+    # builds a B-tree whose leftmost column is ``session_id``, which
+    # covers both ``WHERE session_id = ?`` lookups and the
+    # ``ORDER BY number`` scan used by ``list_questions``.
     op.create_table(
         "questions",
         sa.Column("session_id", sa.Uuid(), nullable=False),
@@ -67,15 +78,8 @@ def upgrade() -> None:
             name="uq_questions_session_number",
         ),
     )
-    op.create_index(
-        "ix_questions_session_id",
-        "questions",
-        ["session_id"],
-        unique=False,
-    )
 
 
 def downgrade() -> None:
-    op.drop_index("ix_questions_session_id", table_name="questions")
     op.drop_table("questions")
     op.drop_column("sessions", "total_questions")

apps/api/src/helprs/core/dependencies.py

@@ -37,14 +37,29 @@ async def get_current_user(
     session: DbSession,
     settings: GetSettings,
 ):
-    """Extract and validate Bearer token, return authenticated GitHubUser."""
+    """Extract and validate Bearer token, return authenticated GitHubUser.
+
+    Token sources, in priority order:
+
+    1. ``Authorization: Bearer <token>`` header — preferred for all
+       normal API calls (apiFetch sets it).
+    2. ``?access_token=<token>`` query parameter — fallback used by SSE
+       endpoints. ``EventSource`` cannot set custom request headers, so
+       the SSE caller must put the JWT in the URL. The query-param path
+       was added 2026-04-11 alongside the Story 3-3 SSE manual-QA fix.
+       Trade-off: query params land in access logs and browser history,
+       which is acceptable for a 30-min JWT but not ideal — preferred
+       long-term solution is fetch+ReadableStream (deferred).
+    """
     from helprs.modules.identity.models import GitHubUser
 
     auth_header = request.headers.get("Authorization")
-    if not auth_header or not auth_header.startswith("Bearer "):
-        raise UnauthorizedError("Missing or invalid Authorization header")
-
-    token = auth_header.removeprefix("Bearer ")
+    if auth_header and auth_header.startswith("Bearer "):
+        token = auth_header.removeprefix("Bearer ")
+    else:
+        token = request.query_params.get("access_token") or ""
+        if not token:
+            raise UnauthorizedError("Missing or invalid Authorization header")
 
     try:
         payload = decode_access_token(token, settings.SECRET_KEY)

apps/api/src/helprs/modules/comprehension/infrastructure/diff_refs.py

@@ -62,15 +62,39 @@ def parse_diff_file_paths(diff: str) -> list[str]:
 def extract_file_refs(diff: str, text: str) -> list[str]:
     """Return the list of file paths from ``diff`` mentioned in ``text``.
 
-    Matching is a plain substring check — cheap, unambiguous, and
-    good enough for UX-DR6's "highlight if the question mentions a
-    file" behaviour. A future Story 3.5 may replace this with an
-    LLM-side tool-call that emits references structurally.
+    Matching uses path-character-aware boundaries so that:
+
+    * ``foo.py`` does NOT match a question mentioning ``foo.py.bak``
+      (the trailing ``.bak`` would extend the path — a separate file)
+    * ``foo.py`` DOES match a sentence ending in ``foo.py.`` (the
+      trailing period is sentence punctuation, not a path extension)
+    * ``src/foo.ts`` does NOT accidentally match a bare ``foo.ts`` in
+      some other directory
+    * ``bar.py`` does NOT match ``bar.pyramid`` (``r`` extends the
+      name into a different identifier)
 
     Order follows diff order so the frontend's "first reference"
-    lookup is deterministic.
+    lookup is deterministic. A future Story 3.5 may replace this
+    with an LLM-side tool-call that emits references structurally.
     """
     if not text or not diff:
         return []
     paths = parse_diff_file_paths(diff)
-    return [p for p in paths if p in text]
+    refs: list[str] = []
+    for path in paths:
+        # Left boundary ``(?<![\w/.-])``: the position must not be
+        # preceded by a path-component character (including ``.`` and
+        # ``/``) so we don't match the tail of a longer path.
+        #
+        # Right boundary ``(?![\w/-])``: the position must not be
+        # followed by a word/slash/dash character (extending the name
+        # or adding another path segment).
+        #
+        # Right boundary ``(?!\.\w)``: the position must not be
+        # followed by ``.<wordchar>`` — this is the key asymmetry that
+        # lets ``foo.py`` match ``foo.py.`` (sentence period) but NOT
+        # ``foo.py.bak`` (path extension).
+        pattern = rf"(?<![\w/.-]){re.escape(path)}(?![\w/-])(?!\.\w)"
+        if re.search(pattern, text):
+            refs.append(path)
+    return refs

apps/api/src/helprs/modules/comprehension/infrastructure/models.py

@@ -13,7 +13,7 @@
 
 import uuid
 
-from sqlalchemy import BigInteger, ForeignKey, Index, Integer, String, UniqueConstraint
+from sqlalchemy import BigInteger, ForeignKey, Integer, String, UniqueConstraint
 from sqlalchemy.orm import Mapped, mapped_column
 
 from helprs.core.database import Base
@@ -60,12 +60,14 @@ class SessionModel(Base):
     status: Mapped[str] = mapped_column(String(20), default="pending", nullable=False)
     # Story 3.3: count of questions the session plans to ask (set once at
     # session creation time by ``StartSessionHandler`` using
-    # ``estimate_question_count``). ``server_default='0'`` is critical —
-    # existing rows must get a value during the backfill migration.
+    # ``estimate_question_count``). The migration uses a transient
+    # ``server_default='0'`` to backfill existing rows, then drops it
+    # so the application layer is the only source of truth. The
+    # Python-side ``default=0`` is kept as a belt-and-braces fallback
+    # for tests that build sessions without specifying a value.
     total_questions: Mapped[int] = mapped_column(
         Integer,
         default=0,
-        server_default="0",
         nullable=False,
     )
 
@@ -82,6 +84,11 @@ class QuestionModel(Base):
     ``SqlAlchemySessionRepository.append_question`` uses a
     ``SELECT ... FOR UPDATE`` lock on the sessions row to make the
     per-session number assignment atomic under concurrent streams.
+
+    Note: no separate single-column index on ``session_id`` — the unique
+    constraint above already builds a B-tree whose leftmost column is
+    ``session_id``, which covers both ``WHERE session_id = ?`` lookups
+    and the ``ORDER BY number`` scan used by ``list_questions``.
     """
 
     __tablename__ = "questions"
@@ -91,7 +98,6 @@ class QuestionModel(Base):
             "number",
             name="uq_questions_session_number",
         ),
-        Index("ix_questions_session_id", "session_id"),
     )
 
     session_id: Mapped[uuid.UUID] = mapped_column(

apps/api/src/helprs/modules/comprehension/presentation/sse.py

@@ -152,12 +152,13 @@ async def generator() -> AsyncIterator[bytes]:
             # it in memory for the duration of the stream.
             previous_texts: list[str] = []
 
-            for number in range(already_generated + 1, total + 1):
+            client_gone = False
+            for _loop_number in range(already_generated + 1, total + 1):
                 # Give up early if the client went away (back button,
                 # tab close). Checked every loop iteration so we
                 # stop minting LLM tokens the user cannot see.
                 if await request.is_disconnected():
-                    await logger.ainfo("sse_stream_client_disconnected", number=number)
+                    await logger.ainfo("sse_stream_client_disconnected", number=_loop_number)
                     return
 
                 question_id = str(uuid.uuid4())
@@ -168,25 +169,54 @@ async def generator() -> AsyncIterator[bytes]:
                     previous_questions=previous_texts,
                     api_key=api_key,
                 ):
+                    # P10: check disconnect inside the token loop so we
+                    # stop streaming LLM tokens immediately when the
+                    # client goes away, not just between questions.
+                    # This matters because a single question can burn
+                    # thousands of BYOK-billed tokens.
+                    if await request.is_disconnected():
+                        await logger.ainfo(
+                            "sse_stream_client_disconnected_mid_token",
+                            number=_loop_number,
+                            tokens_streamed=len(parts),
+                        )
+                        client_gone = True
+                        break
                     parts.append(token)
                     yield _sse_frame(
                         "question_token",
                         {
                             "question_id": question_id,
                             "token": token,
-                            "number": number,
+                            "number": _loop_number,
                             "total": total,
                         },
                     )
 
+                if client_gone:
+                    # Do NOT persist a half-streamed question. The
+                    # next reconnection (if any) will start fresh from
+                    # ``already_generated`` which the DB still reports.
+                    return
+
+                # P11: reject empty LLM output rather than persisting a
+                # row with ``sha256("")``. An empty question is never a
+                # legitimate outcome — surface it as an error frame.
+                if not parts:
+                    raise RuntimeError("LLM yielded no tokens for question")
+
                 text = "".join(parts)
                 text_hash = hashlib.sha256(text.encode("utf-8")).hexdigest()
                 file_refs = extract_file_refs(diff, text)
 
                 # Fresh short-lived session per commit — the
                 # request-scoped ``db`` is intentionally NOT reused.
                 async with get_db_context() as tx:
-                    await SqlAlchemySessionRepository(tx).append_question(
+                    # P8: use the DB-assigned number from append_question
+                    # rather than the Python loop variable. Under concurrent
+                    # streams the assigned number may diverge from the loop
+                    # variable (see two-tab scenario in review findings D1).
+                    persisted = await SqlAlchemySessionRepository(tx).append_question(
                         session_id=session_id,
                         topic=Topic.ARCHITECTURE,  # TODO(story-3.5): topic selection
                         text_hash=text_hash,
@@ -199,23 +229,31 @@ async def generator() -> AsyncIterator[bytes]:
                     {
                         "question_id": question_id,
                         "text": text,
-                        "number": number,
+                        "number": persisted.number,
                         "total": total,
                         "file_refs": file_refs,
                     },
                 )
                 await logger.ainfo(
                     "sse_stream_question_committed",
-                    number=number,
+                    number=persisted.number,
                     total=total,
                     file_refs_count=len(file_refs),
                 )
 
+            # P9: report the actual persisted count rather than the
+            # target ``total``. Opens a fresh short-lived session so
+            # we don't reach back into ``db`` after the DB phase.
+            async with get_db_context() as tx:
+                actual_count = await SqlAlchemySessionRepository(tx).count_questions(
+                    session_id=session_id,
+                )
+
             yield _sse_frame(
                 "done",
-                {"session_id": session_id_str, "question_count": total},
+                {"session_id": session_id_str, "question_count": actual_count},
             )
-            await logger.ainfo("sse_stream_done", total=total)
+            await logger.ainfo("sse_stream_done", question_count=actual_count, total=total)
 
         except asyncio.CancelledError:
             # Client disconnect surfaced via task cancellation — log

apps/api/src/helprs/modules/identity/router.py

@@ -20,7 +20,12 @@
 router = APIRouter(prefix="/auth", tags=["auth"])
 
 GITHUB_AUTHORIZE_URL = "https://github.com/login/oauth/authorize"
-OAUTH_SCOPES = "read:user,user:email"
+# ``read:org`` is required so ``get_installations_for_user`` can call
+# ``GET /user/orgs`` to verify membership for Org-type installs (see
+# ``installation/service.py::get_installations_for_user``). User-type
+# installs do not need it. Added 2026-04-11 alongside the Epic-1
+# ``/user/installations`` regression fix surfaced by Story 3-3 QA.
+OAUTH_SCOPES = "read:user,user:email,read:org"
 
 
 @router.get("/github")