Skip to content

Address remaining low-severity code review findings (#31, #32, #34, #36, #38, #39, #41, #42, #44, #47, #48)#115

Merged
pambrose merged 3 commits into
masterfrom
fix-lows-batch-3
Jun 15, 2026
Merged

Address remaining low-severity code review findings (#31, #32, #34, #36, #38, #39, #41, #42, #44, #47, #48)#115
pambrose merged 3 commits into
masterfrom
fix-lows-batch-3

Conversation

@pambrose

Copy link
Copy Markdown
Contributor

Addresses the remaining 11 open low-severity findings from the multi-agent code review (docs/CODE_REVIEW_JUNE_2026.md), bringing it to 48 of 48 addressed.

Findings fixed

Reliability / correctness

  • 1.19.0 #31 extractJavaFunction — accept ChallengeName and guard the < 2 static case with a named error instead of an opaque NoSuchElementException/IllegalArgumentException during content load.
  • 2.1.1 #47 pythonAdjust/parseListElements — quote-aware splitTopLevelCommas tokenizer so commas inside quoted list elements no longer split; unquote both single and double quotes.
  • 1.28.0 #39 Background content load — wrapped in runInitialContentLoad, which catches and logs failures (with stack trace) so a load exception no longer leaves the server permanently "not ready".
  • Migrate authentication from form-based to OAuth #48 evalContent — log the failing contentSource.source instead of $this.

Resource / performance

  • 1.25.0 #36 geoInfoMap — bounded LRU cache (MAX_GEO_CACHE_SIZE); transient API failures persist a blank placeholder row (so the server_requests.geo_ref FK resolves) but are kept out of the in-memory cache so they retry instead of being permanently poisoned.
  • 1.27.0 #38 Request logging — coalesced session lookup, geo-id resolution, and insert into a single transaction (geo HTTP lookup stays outside), cutting per-request pool checkouts from 3-4 to one.
  • 1.20.0 #32 Per-user answer channels — replaced the unbounded ConcurrentHashMap<userId, Channel> with a fixed-size worker pool keyed by userDbmsId, preserving per-user serialization.

Error handling / design

  • 1.23.0 #34 WS validation failures now log the cause and close with VIOLATED_POLICY (shared WsCommon.rejectInvalidWsRequest) instead of a generic GOING_AWAY, across all four teacher-facing handlers.
  • 1.30.0 #41 Extracted PageUtils.wsHostRewriteJs() (6 origin-rewrite sites) and summaryOnMessageJs() (Class/Student summary onmessage loops); fixed a misaligned brace.
  • 1.31.0 #42 Split displayStudentProgress into a data pass (EnrolleeProgress, answerHistoryBulk out of the render loop) and a pure render pass; extracted server-side color helpers.
  • 1.35.0 #44 Safe per-property init guard: isInitialized() = global flag OR this property individually set (strictly more permissive than the old global flag). The full per-property throwing guard remains declined as unsafe; no functional bug.

Tests

New Kotest specs run locally: JavaFunctionExtractTest, PythonListTokenizerTest, InitialContentLoadTest, GeoCacheBoundedTest, UserAnswerQueueTest, PageUtilsWsTest, PropertyInitGuardTest. DB-backed GeoCacheTest updated for the new failure semantics (verified on CI).

Also: review-doc reorg (CODE_REVIEW_SUGGESTIONS.mdCODE_REVIEW_MARCH_2026.md, findings → CODE_REVIEW_JUNE_2026.md) and an OAuthSessionRotationTest tidy.

🤖 Generated with Claude Code

pambrose and others added 3 commits June 14, 2026 11:48
#31 — extractJavaFunction: accept ChallengeName and guard the < 2 static
declaration case with a named IllegalStateException instead of an opaque
NoSuchElementException / IllegalArgumentException during content load.

#47 — Add a quote-aware splitTopLevelCommas tokenizer and use it in both
parseListElements and the pythonAdjust formatter so commas inside quoted
list elements no longer split, and unquote both single and double quotes.

#39 — Wrap the background content load in runInitialContentLoad, which
catches and logs any failure (with stack trace) so a load exception no
longer silently leaves the server permanently "not ready".

#36 — Bound geoInfoMap with an LRU LinkedHashMap (MAX_GEO_CACHE_SIZE) and
stop caching/persisting transient API failures: success persists + re-reads
(cacheable), failure returns an un-persisted Unknown placeholder that is not
cached, so failures retry instead of permanently poisoning the cache.

Tests: JavaFunctionExtractTest, PythonListTokenizerTest, InitialContentLoadTest,
GeoCacheBoundedTest (all run locally); GeoCacheTest rewritten for the new
DB-backed failure semantics (CI-verified).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#32 — Replace the unbounded per-user answer-channel map with a fixed-size
worker pool keyed by userDbmsId, so channels/coroutines stay bounded while
each user's writes remain serialized.

#34 — Catch InvalidRequestException in the four teacher-facing WS handlers
and close with VIOLATED_POLICY via a shared WsCommon.rejectInvalidWsRequest
helper, instead of a generic GOING_AWAY with no logged cause.

#38 — Coalesce the request-logging session lookup, geo-id resolution, and
insert into a single transaction (geo HTTP lookup stays outside it), cutting
per-request pool checkouts from 3-4 to one.

#41 — Extract PageUtils.wsHostRewriteJs() (applied to all six origin-rewrite
sites) and summaryOnMessageJs(prefixField) (collapses the Class/Student
summary onmessage loops), and fix the misaligned brace in StudentSummaryPage.

#42 — Split displayStudentProgress into a data pass (EnrolleeProgress rows,
answerHistoryBulk out of the render loop) and a pure render pass; extract
answerCellColor/nameCellColor server-side color helpers.

#44 — Track per-property initialization (setProperty records the name) and
have the read guards check isInitialized() = global flag OR this property
individually set. Strictly more permissive than the old global flag, so it
never throws where the old guard didn't, while making the property-named
error meaningful. The full per-property throwing guard remains declined as
unsafe; the finding notes no functional bug.

#48 — Log the failing contentSource.source (not $this) when DSL evaluation
fails in evalContent.

Also reconciles #36 with #38: server_requests.geo_ref is a non-nullable FK,
so a failed geo lookup now persists a blank placeholder row (FK resolves) but
is kept out of the in-memory cache, so it is not poisoned and retries later.

Tests: UserAnswerQueueTest, PageUtilsWsTest, PropertyInitGuardTest (run
locally); GeoCacheTest updated for the new DB-backed failure semantics
(CI-verified).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add docs/CODE_REVIEW_JUNE_2026.md (the multi-agent review findings, now 48
  of 48 addressed) and rename docs/CODE_REVIEW_SUGGESTIONS.md to
  docs/CODE_REVIEW_MARCH_2026.md.
- OAuthSessionRotationTest: drop unused imports and redundant !! assertions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pambrose pambrose merged commit bb63e1e into master Jun 15, 2026
3 of 4 checks passed
@pambrose pambrose deleted the fix-lows-batch-3 branch June 15, 2026 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant