Merged
Conversation
Contributor
Greptile SummaryThis PR establishes the project's code quality foundation: Biome replaces Prettier (single tool for lint + format + import sorting), a Husky pre-commit hook enforces incremental cleanup on staged files, a GitHub Actions CI runs parallel lint and test jobs, and 58 new unit tests cover 8 core utility modules. CLAUDE.md is replaced with a symlink to the new Confidence Score: 5/5Safe to merge — all findings are P2 style/workflow concerns with no impact on runtime behaviour. No P0 or P1 issues. The three P2 findings are: (1) --error-on-warnings may occasionally block commits for non-fixable warnings, (2) lint-format CI job is skipped on direct pushes to main, and (3) a misleading comment in the authResolver tests. None affect correctness, data integrity, or security. .husky/pre-commit and .github/workflows/ci.yml are the two files worth a second look for the workflow concerns noted above.
|
| Filename | Overview |
|---|---|
| .github/workflows/ci.yml | New CI pipeline: lint-format (PR only) + test jobs; unquoted $FILES variable and lint job skipped on direct pushes to main/dev |
| .husky/pre-commit | Pre-commit hook runs biome on staged files; --error-on-warnings may block commits when non-auto-fixable warnings exist (e.g. noUnusedVariables) |
| biome.json | Biome 2.4.11 config replacing Prettier; minimal ruleset with recommended rules off, correctness/style/security rules explicitly set |
| package.json | Removes Prettier/plugins, adds @biomejs/biome and husky as devDependencies; adds biome lint/format/style scripts and prepare hook |
| src/middleware/tests/authResolver.test.ts | New auth tests via resolveAuth; dynamic imports used with misleading 'pick up env changes' comment — module is actually cached by Bun |
| src/utils/tests/planningJsonExtractor.test.ts | Tests all extraction strategies (direct JSON, markdown code block, surrounding text, fallback) plus field normalization |
| src/utils/tests/state.test.ts | Comprehensive tests for addVariablesToState, cleanWebSearchResults, formatConversationHistory, and parseKeyValueXml |
| AGENTS.md | New agent-optimised instructions (CLAUDE.md symlinked here); consolidates commands, structure, running modes, and behavioral mandates |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Developer edits code] --> B[git commit]
B --> C{Staged TS/JS/JSON/CSS files?}
C -- No --> D[Commit proceeds]
C -- Yes --> E[biome check staged and write]
E --> F{Warnings remaining?}
F -- Yes --> G[Commit BLOCKED by error-on-warnings]
F -- No --> H[git update-index again]
H --> D
I[Push or Open PR] --> J[GitHub Actions CI]
J --> K[test job: typecheck + bun test]
J --> L{PR event?}
L -- Yes --> M[lint-format: biome ci changed files]
L -- Direct push to main/dev --> N[lint-format SKIPPED]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .husky/pre-commit
Line: 5
Comment:
**`--error-on-warnings` may block commits for non-auto-fixable warnings**
`biome.json` marks `noUnusedVariables` and `noDangerouslySetInnerHtml` as `"warn"`. `biome check --write` cannot auto-remove unused variables, so any staged file that triggers those warnings will leave warnings behind, and `--error-on-warnings` will abort the commit. Developers touching existing files with unused variables (even in unrelated lines) will be blocked until they clean up those symbols.
Consider dropping `--error-on-warnings` or upgrading those rules to `"error"` so the intent is explicit and the experience is predictable:
```suggestion
biome check --staged --write
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 27-31
Comment:
**Unquoted `$FILES` and lint skipped on direct pushes**
Two independent concerns on this step:
1. `$FILES` is unquoted — if any path contains spaces or glob characters it will word-split incorrectly. Quote it: `biome ci $FILES` → `biome ci "$FILES"` (or pass via `xargs`).
2. The `lint-format` job has `if: github.event_name == 'pull_request'`, so a direct push to `main` or `dev` skips linting entirely, making it possible to land unformatted code without CI blocking it.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/middleware/__tests__/authResolver.test.ts
Line: 17-19
Comment:
**Dynamic import doesn't produce a fresh module in Bun**
The comment says "Dynamic import to pick up env changes", but Bun (like Node) caches modules after the first resolution. All four tests receive the same cached `authResolver` instance regardless of when the `import()` call is made. The tests still pass because `resolveAuth` reads `process.env` at call time (consistent with the TDZ guidelines in CLAUDE.md), so a static top-level import would work equally well and be clearer. The misleading comment could cause confusion if env-dependent module-level initialisation is added later.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add basic code quality infrastructure" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.(js|jsx|ts|tsx|json|css)$' || true) | ||
|
|
||
| if [ -n "$STAGED_FILES" ]; then | ||
| biome check --staged --error-on-warnings --write |
Contributor
There was a problem hiding this comment.
--error-on-warnings may block commits for non-auto-fixable warnings
biome.json marks noUnusedVariables and noDangerouslySetInnerHtml as "warn". biome check --write cannot auto-remove unused variables, so any staged file that triggers those warnings will leave warnings behind, and --error-on-warnings will abort the commit. Developers touching existing files with unused variables (even in unrelated lines) will be blocked until they clean up those symbols.
Consider dropping --error-on-warnings or upgrading those rules to "error" so the intent is explicit and the experience is predictable:
Suggested change
| biome check --staged --error-on-warnings --write | |
| biome check --staged --write |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .husky/pre-commit
Line: 5
Comment:
**`--error-on-warnings` may block commits for non-auto-fixable warnings**
`biome.json` marks `noUnusedVariables` and `noDangerouslySetInnerHtml` as `"warn"`. `biome check --write` cannot auto-remove unused variables, so any staged file that triggers those warnings will leave warnings behind, and `--error-on-warnings` will abort the commit. Developers touching existing files with unused variables (even in unrelated lines) will be blocked until they clean up those symbols.
Consider dropping `--error-on-warnings` or upgrading those rules to `"error"` so the intent is explicit and the experience is predictable:
```suggestion
biome check --staged --write
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+27
to
+31
| run: | | ||
| FILES=$(git diff --name-only --diff-filter=ACMR "${{ github.event.pull_request.base.sha }}" HEAD -- '*.ts' '*.tsx' '*.js' '*.jsx' '*.json' '*.css' | tr '\n' ' ') | ||
| if [ -n "$FILES" ]; then | ||
| biome ci $FILES | ||
| fi |
Contributor
There was a problem hiding this comment.
$FILES and lint skipped on direct pushes
Two independent concerns on this step:
-
$FILESis unquoted — if any path contains spaces or glob characters it will word-split incorrectly. Quote it:biome ci $FILES→biome ci "$FILES"(or pass viaxargs). -
The
lint-formatjob hasif: github.event_name == 'pull_request', so a direct push tomainordevskips linting entirely, making it possible to land unformatted code without CI blocking it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 27-31
Comment:
**Unquoted `$FILES` and lint skipped on direct pushes**
Two independent concerns on this step:
1. `$FILES` is unquoted — if any path contains spaces or glob characters it will word-split incorrectly. Quote it: `biome ci $FILES` → `biome ci "$FILES"` (or pass via `xargs`).
2. The `lint-format` job has `if: github.event_name == 'pull_request'`, so a direct push to `main` or `dev` skips linting entirely, making it possible to land unformatted code without CI blocking it.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+17
to
+19
|
|
||
| // Dynamic import to pick up env changes | ||
| const { resolveAuth } = await import("../authResolver"); |
Contributor
There was a problem hiding this comment.
The comment says "Dynamic import to pick up env changes", but Bun (like Node) caches modules after the first resolution. All four tests receive the same cached authResolver instance regardless of when the import() call is made. The tests still pass because resolveAuth reads process.env at call time (consistent with the TDZ guidelines in CLAUDE.md), so a static top-level import would work equally well and be clearer. The misleading comment could cause confusion if env-dependent module-level initialisation is added later.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/middleware/__tests__/authResolver.test.ts
Line: 17-19
Comment:
**Dynamic import doesn't produce a fresh module in Bun**
The comment says "Dynamic import to pick up env changes", but Bun (like Node) caches modules after the first resolution. All four tests receive the same cached `authResolver` instance regardless of when the `import()` call is made. The tests still pass because `resolveAuth` reads `process.env` at call time (consistent with the TDZ guidelines in CLAUDE.md), so a static top-level import would work equally well and be clearer. The misleading comment could cause confusion if env-dependent module-level initialisation is added later.
How can I resolve this? If you propose a fix, please make it concise.
raghav-rama
reviewed
Apr 13, 2026
- openai: ^6.6.0 → ^6.34.0 - @anthropic-ai/sdk: ^0.66.0 → ^0.90.0 Both SDKs now include proper type definitions for web_search tools, reasoning_effort, ContentBlockParam, and other features that were previously worked around with @ts-ignore directives.
- Close unclosed if-block in sendDeepResearchMessage (line 298) that caused 3 cascading compilation errors - Add missing return statement for the deep research response - Change catch clauses from `err: any` to `err: unknown`
- Scope root tsconfig to src/ only; client/ and documentation/ have their own tsconfigs - Enable strict mode in client/tsconfig.json - Add @types/papaparse for CSV parser types - Fix implicit any params (parsers.ts, subscribe.ts) - Fix possibly-undefined access (openrouter.ts, chat.worker.ts) - Fix state.ts endStep missing required field - Add Zod validation at JSON.parse boundaries (edison.ts) - Remove stale @ts-ignore directives (canvas-polyfill.ts, edison.ts) - Use declare global for globalThis polyfill augmentation - Simplify fetchWithRetry to accept string | URL only (no callers pass Request)
- Type DB operation params with StateValues/ConversationStateValues instead of any; add DbState/DbConversationState interfaces - Add parseLLMProviderName runtime validator, eliminate 5 @ts-ignore on LLM constructor calls across agents and workers - Replace property-stuffing on Error with FallbackError class; use instanceof checks instead of (error as any).requiresFallback - Make LLMAdapter.logDuration public to remove (this.adapter as any) casts in LLM provider - Change abstract transformRequest/transformResponse signatures from any to unknown - Add error/status fields to StateValues
- Add ElysiaRouteContext<TParams, TBody> in src/types/elysia.ts - Add global Request augmentation so request.auth is typed everywhere - Apply ElysiaRouteContext to 12 route handlers replacing ctx: any - Remove (request as any).auth casts; use request.auth directly - Replace error: any catches with unknown + instanceof narrowing - Introduce PaperStatusResponse type for response building in paperStatusHandler - Narrow FormData body via type-guard helpers instead of as casts
Four principles for agents working on the repo: - Think before coding: state ambiguity, don't guess - Simplicity first: no features beyond what was asked - Surgical changes: every line traces to the request - Goal-driven execution: tests define done
- Replace ws.data mutation pattern with per-client WeakMap<WsClient, WsConnectionState> keyed by the ws instance - Define WsClient as a minimal structural interface (send, close, readyState) and use Elysia's ws via structural assignment - Add IncomingMessage discriminated union (auth/subscribe/ unsubscribe/ping) with a parseIncomingMessage runtime guard - Remove all (ws.data as any) casts and Set<any>/Map<any, ...> state - Properly handle readyState undefined case in cleanupDeadConnections
- Use SDK-provided types directly for OpenAI web_search tool, reasoning_effort, and ResponseStreamEvent discriminated union - Type LLMRequest.format as ResponseFormatTextConfig from openai SDK - Split extractSources logic to use typed ResponseFunctionWebSearch and its Search action shape - Replace @ts-ignore on OpenRouter usage token fields with unified field access across usage/metrics.tokens shapes - Validate OpenRouter responses via Zod schema at the fetch boundary instead of casting the parsed JSON - Remove (anthropicRequest as any) on adaptive thinking and output_config (now typed in @anthropic-ai/sdk 0.90.0)
- Convert logger.error(msg, error as any) to pino's idiomatic
logger.error({ err }, msg) in fileUpload/index.ts,
embeddings/documentProcessor.ts, embeddings/vectorSearchWithDocs.ts,
and storage/providers/s3.ts
- Replace (globalThis as any).__knowledgeVectorSearch with typed
global augmentation via declare global
- Type getToolDefinitions() return as Tool[] from Anthropic SDK so
it matches MessageCreateParams.tools without a cast
- Use request.auth directly in rateLimiter.ts now that Request is
globally typed via ElysiaRouteContext module augmentation
- Replace (discovery as any).jobId with evidenceArray-first lookup
in paper/prompts.ts; use property-in narrowing in generatePaper.ts
for legacy discovery fallback
- Replace PAPER_GEN_LLM_PROVIDER cast with parseLLMProviderName
validator
- Type Tool.execute with Message and unknown instead of any
- Type UploadedFile.metadata, TextChunk.metadata, and
VectorSearchResult.metadata as Record<string, unknown>
- Type addVariablesToState with StateValues/ConversationStateValues
overloads; type formatConversationHistory with a minimal message
shape instead of any[]
- Narrow catch (err: any) to catch (err: unknown) + instanceof Error
in chat/setup, files/description, files/index, jwt, paper/utils,
storage/s3, embeddings/*, and LLM adapters
- Type middleware set/body params with concrete shapes
({ status?; headers }, body?: unknown)
- Type ConversationSetup records with DbState / DbConversationState
- Type createdMessage in deep-research/start via an alias matching
createMessage's return shape
- Use the Anthropic Tool[] type for getToolDefinitions so
MessageCreateParams.tools matches without a cast
- Parse OpenRouter response bodies from unknown via Zod / structural
narrowing instead of as casts
- Retain a single documented 'messageContent: any' in generatePaper
where widening LLMRequest.content to support Anthropic vision
blocks would cascade through every adapter's SDK message type
- Add null guards for noUncheckedIndexedAccess on array/map access (build.ts env parser, useSessions realtime handlers, ChatPage optional research state chains, parseCitations regex matches, helpers UUID byte access) - Replace React.CSSProperties with JSX.CSSProperties from preact in SuggestedSteps - Narrow AuthContext.decodeJWTPayload via local variable + null check before atob() - Handle File|undefined in useFileUpload, usePresignedUpload with explicit guards and ?? null defaults - Add updated_at to client Message interface (already set by DB) - Add sidecar .d.ts files for .jsx components (ChatInput, ErrorMessage, LoginScreen, Message, Sidebar, TypingIndicator, WelcomeScreen) with properly-typed Preact component signatures based on actual call sites
- Type IconButton onClick handler with Preact's TargetedMouseEvent - Type useAutoScroll dependencies as readonly unknown[] - Narrow useChatAPI poll result files via ChatResponse['files'] - Add MessageFileMetadata interface in lib/supabase and use for both dbMsg.files and Message.files - Type ChatPage callbacks with Message/DBMessage from supabase and useSessions; catch err as unknown with instanceof narrowing - Type state?: Record<string, unknown> on client Message interface
Replace Prettier with Biome for linting, formatting, and import sorting. Add Husky pre-commit hook that runs Biome on staged files incrementally. Add GitHub Actions CI with lint-format (PR changed files only) and test jobs. Add initial test suites (58 tests) for core utilities: uuid, cache, planningJsonExtractor, state, discovery, DOI, textUtils, authResolver. Rewrite CLAUDE.md into concise agent-optimized AGENTS.md with symlink for backwards compatibility.
maschlr
force-pushed
the
ms-add-basic-code-quality
branch
from
e0c1dd0 to
f4baf3e
Compare
- Restore rawFiles buffer/parsedText strip in cleanValues to migrate
legacy Supabase state docs on next write (extracted to cleanValues.ts
for testability).
- Replace `{ ...error }` spreads in setup.ts and description.ts with
`instanceof Error` narrowing; the spread produced `{}` for standard
Errors because message/code/stack are non-enumerable, breaking the
23505 race-detection path and hiding real PDF/OCR/Excel extraction
errors in logs.
- Chat worker logs a warning and skips the wait when the file-process
queue is unavailable rather than silently proceeding.
- WebSocket cleanup treats undefined readyState as dead so connections
from implementations that don't expose readyState are not leaked.
- LLM adapters (openrouter-alpha, edison) use Zod safeParse so provider
schema drift surfaces as a logged warning / structured error instead
of a ZodError that bypasses the FallbackError retry path.
- Export getState and parseIncomingMessage from the WebSocket handler
to enable unit testing.
- retry.test.ts covers FallbackError shape, isRetryableError patterns, backoff delay bounds, fallback-config lookup, and withRetry paths (success, retryable, non-retryable, fallback-triggered, enableFallback disabled, and onRetry/onFallback callbacks). - websocket handler.test.ts covers parseIncomingMessage (non-JSON, primitive JSON, missing/bad action, auth/subscribe/unsubscribe/ping shapes with field-type guards) and WeakMap-based state isolation. - cleanValues.test.ts exercises all strip branches including the legacy rawFiles buffer/parsedText migration, input immutability, and empty/ malformed-entry handling. - authResolver.test.ts gains proper beforeEach/afterEach env restore (deleting when original was undefined), JWT happy-path + wrong-secret + expired cases using jose HS256 signing, constant-time-compare length-mismatch and same-length-wrong-value coverage, and strict-mode unauthenticated-rejection case.
- openrouter adapter warns on empty response.choices before returning
"" so that an error-like 200 response is visible in logs instead of
being treated as "no content".
- fetchWithRetry JSDoc documents the string|URL narrowing so future
Request callers widen the type intentionally (and restore .clone()
on retry) rather than silently reusing a consumed body.
- Deep-research status reader filters malformed legacy rawFiles
(missing filename/mimeType) instead of emitting {filename:"",
mimeType:""} entries that would mask corrupted state.
- Move isBodyRecord, asString, and extractFiles from chat.ts and deep-research/start.ts into src/utils/bodyParsing.ts; both routes now import from the shared module. - Move the CreatedMessage type and initKnowledgeBase() call below imports in deep-research/start.ts per the module-level-imports rule. - Drop the unused [key: string]: unknown index signature on Tool.execute input and the unused TBody generic on ElysiaRouteContext; both were escape hatches the type-strict refactor no longer needs. - Remove pre-Biome eslint-disable comments in canvas-polyfill.ts.
Extract the stability-critical logic behind four post-review fixes into pure functions so they can be tested without a running environment. - cleanupDeadConnectionsIn(map) takes the connection map as a parameter; cleanupDeadConnections() is the zero-arg wrapper the cron caller uses. New tests cover OPEN/CLOSING/CLOSED/undefined readyState cases and empty-set conversation cleanup — including the regression fix where undefined readyState must be treated as dead. - openrouter-extractors.ts now owns extractText / extractWebSearchResults and the OpenRouterResponse shape. Tests exercise empty-choices warn, URL-citation parsing, dedup, and skipped annotation types. - parseOpenRouterResponse is the exported safeParse helper used by openrouter-alpha; tests cover valid, schema-mismatch (raw pass-through + warn), missing-url context, null input, and all-optional-fields. - waitForPendingFiles is extracted from chat.worker.ts with injected queue, status reader, and sleep. Tests cover the regression fix (queue=null + pending files logs warn and returns), completed/ready/ missing-job happy paths, failed/error file paths, multi-file iteration, and maxWaitMs bound.
4 tasks
raghav-rama
approved these changes
Apr 22, 2026
maschlr
added a commit
that referenced
this pull request
Apr 22, 2026
Rebase the report on dev after #156 and replace stale claims with verified evidence from the current codebase. Add a prioritized action matrix so remediation work can be sequenced by impact, effort, ownership, and dependencies.
maschlr
added a commit
that referenced
this pull request
Apr 22, 2026
* Add backend architecture review document * Update architecture review with current-state findings Rebase the report on dev after #156 and replace stale claims with verified evidence from the current codebase. Add a prioritized action matrix so remediation work can be sequenced by impact, effort, ownership, and dependencies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Broad code-quality pass on the BioAgents server and client. Four main areas:
Tooling & CI
lint-format(PR-changed files) andtest(typecheck +bun:test) jobs.editorconfigfor cross-editor consistencyCODING_GUIDELINES.mddocumenting team principlesAGENTS.mdas the canonical agent-facing instructions;CLAUDE.mdsymlinked for backwards compatibilityTypeScript strict mode
: anyandas anyacrosssrc/andclient/@ts-ignoredirectives in LLM adaptersElysiaRouteContext(src/types/elysia.ts)x402 / b402 payment protocol removed
X402_V2_MIGRATION.mddocuseEmbeddedWallet,useX402Payment),CDPProvider,PaymentConfirmationModal,EmbeddedWalletAuth20260414000000_drop_x402_tables.sqldrops the leftover tables.env.example; auth docs and sidebar trimmed accordinglyError handling & silent-failure fixes
openrouter,fetchWithRetry, and the status reader instead of swallowing errorssrc/utils/bodyParsing.tsand dropped an unused type surfaceTests
New
bun:testcoverage for core utilities and critical paths:src/utils/__tests__/:uuid,cache,state,discovery,planningJsonExtractorsrc/services/paper/utils/__tests__/:doi,textUtilssrc/middleware/__tests__/authResolver.test.tssrc/llm/__tests__/retry.test.tssrc/llm/adapters/__tests__/:openrouter,openrouter-alphasrc/services/websocket/__tests__/handler.test.tssrc/services/queue/workers/__tests__/fileWait.test.tssrc/db/__tests__/cleanValues.test.tsDependency updates
openaiand@anthropic-ai/sdkbumped to latestTest plan
git commit(stage a file, commit, observe Biome output)devto confirm strict-mode refactor introduced no runtime regressions