Feat/tool integration v2 - #86
Conversation
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:2, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:3, coverage:90%)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| if (prMrUrl) { | ||
| const url = prMrUrl.toLowerCase(); | ||
| if (url.includes('github.com')) return 'github'; |
Implement Third-Party Tool IntegrationOverviewThis change introduces a new feature enabling third-party static analysis tools to run as Lambda-backed jobs during LiveReview code reviews. It establishes a comprehensive system including new database schemas, API endpoints, UI components, and job orchestration. The system aims to provide detailed tool findings and credit consumption within the review process. Technical Highlights
Impact
|
|
|
||
| 1. THE System SHALL provide an `available_tools` table with columns: `id` (bigserial primary key), `name` (text not null unique), `description` (text not null), and `lambda_arn` (text not null). | ||
| 2. THE System SHALL seed the `available_tools` table with at least two initial rows: one for `ruff` and one for `pylint`, each with a non-empty `description` and a placeholder `lambda_arn`. | ||
| 3. THE System SHALL manage the `available_tools` schema exclusively through dbmate migration files located in `db/migrations/`, and SHALL NOT apply these migrations directly to any production database. |
There was a problem hiding this comment.
Severity: critical
The dbmate migrations have not been applied to the production environment. The strategy for updating the schema needs to be clarified.
Suggestions:
- Clarify how production database schema updates will be managed if dbmate migrations are not applied directly.
- Reconcile this statement with standard dbmate usage for all environments.
|
|
||
| 1. THE System SHALL provide an `org_tools` table with columns: `org_id` (bigint not null, references `organizations.id`), `tool_id` (bigint not null, references `available_tools.id`), `enabled` (boolean not null default false), `config_json` (jsonb not null default `'{}'`), and a composite primary key on (`org_id`, `tool_id`). | ||
| 2. THE System SHALL enforce that every row in `org_tools` references a valid `org_id` in the `organizations` table and a valid `tool_id` in the `available_tools` table via foreign key constraints. | ||
| 3. THE System SHALL manage the `org_tools` schema exclusively through dbmate migration files and SHALL NOT apply these migrations directly to any production database. |
There was a problem hiding this comment.
Severity: critical
The dbmate migrations have not been applied to the production environment. The strategy for updating the schema needs to be clarified.
Suggestions:
- Clarify how production database schema updates will be managed if dbmate migrations are not applied directly.
- Reconcile this statement with standard dbmate usage for all environments.
| 1. WHEN `isCloudMode()` returns `true` AND the authenticated user's role is `owner`, THE Settings Page SHALL render a navigable tab at the hash route `third-party-tools` within `/#/settings`. | ||
| 2. WHEN `isCloudMode()` returns `false`, THE Settings Page SHALL NOT render the `third-party-tools` tab. | ||
| 3. WHEN the authenticated user's role is not `owner`, THE Settings Page SHALL NOT render the `third-party-tools` tab as a clickble navigation item. | ||
| 4. WHEN a non-owner org member navigates directly to `/#/settings#third-party-tools`, THE Settings Page SHALL render a read-only view of the enabled tools without controls to modify them. |
There was a problem hiding this comment.
Severity: warning
A read-only view should be implemented for non-owners. Ensure that no sensitive configuration information is exposed through this view.
Suggestions:
- Verify that the read-only view for non-owners does not expose any sensitive configuration details that should be restricted.
- Implement robust data filtering on the backend for read-only access.
| id bigserial PRIMARY KEY, | ||
| name text NOT NULL UNIQUE, | ||
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The lambda_arn column is defined as NOT NULL, but the seeding process requires placeholders. This needs to be reconciled with requirement R1.2.
Suggestions:
- Clarify how
lambda_arnwill be handled for initial seeded tools (R1.2) if they areNOT NULLand 'no hardcoded ARNs belong here'. - Consider allowing
NULLinitially or ensuring a robust placeholder/update mechanism.
| name text NOT NULL UNIQUE, | ||
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, | ||
| multiplier numeric(6,2) NOT NULL DEFAULT 1.0, |
There was a problem hiding this comment.
Severity: warning
A multiplier column has been added. Its purpose is not clear from the schema definition.
Suggestions:
- Add a comment or documentation explaining the purpose and usage of the
multipliercolumn.
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, | ||
| multiplier numeric(6,2) NOT NULL DEFAULT 1.0, | ||
| use_case text NOT NULL DEFAULT '', |
There was a problem hiding this comment.
Severity: warning
A use_case column has been added. Its purpose is not clear from the schema definition.
Suggestions:
- Add a comment or documentation explaining the purpose and usage of the
use_casecolumn.
| created_at timestamptz NOT NULL DEFAULT now() | ||
| ); | ||
|
|
||
| -- Tools are registered via the lr-tools deployer's `register-tools` command, |
There was a problem hiding this comment.
Severity: critical
This comment conflicts with requirement R1.2, which specifies seeding with placeholder ARNs.
Suggestions:
- Reconcile this comment with Requirement 1.2, which states that
available_toolsshould be seeded with placeholderlambda_arns. - Clarify if seeding happens outside of this migration or if placeholders are acceptable here.
| @@ -0,0 +1,15 @@ | |||
| -- migrate:up | |||
| CREATE TABLE IF NOT EXISTS public.org_tools ( | |||
| org_id bigint NOT NULL REFERENCES public.orgs(id) ON DELETE CASCADE, | |||
There was a problem hiding this comment.
Severity: warning
The ON DELETE CASCADE option is used for the orgs table. Please confirm that this aligns with the business logic requirements.
Suggestions:
- Confirm that cascading deletion of
org_toolsentries uponorgsdeletion aligns with business requirements. - Consider
ON DELETE RESTRICTorSET NULLiforg_toolsdata needs to be preserved or handled differently.
| -- migrate:up | ||
| CREATE TABLE IF NOT EXISTS public.org_tools ( | ||
| org_id bigint NOT NULL REFERENCES public.orgs(id) ON DELETE CASCADE, | ||
| tool_id bigint NOT NULL REFERENCES public.available_tools(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
Severity: warning
The ON DELETE CASCADE option is used for the available_tools table. Please confirm that this aligns with the business logic requirements.
Suggestions:
- Confirm that cascading deletion of
org_toolsentries uponavailable_toolsdeletion aligns with business requirements. - This is generally acceptable, as configurations for a non-existent tool are usually irrelevant.
| tool_id bigint NOT NULL REFERENCES public.available_tools(id) ON DELETE CASCADE, | ||
| enabled boolean NOT NULL DEFAULT false, | ||
| config_json jsonb NOT NULL DEFAULT '{}', | ||
| updated_at timestamptz NOT NULL DEFAULT now(), |
There was a problem hiding this comment.
Severity: warning
The updated_at column lacks an ON UPDATE trigger, meaning it is only set upon insertion and not updated thereafter.
Suggestions:
- Add an
ON UPDATEtrigger to automatically update theupdated_attimestamp whenever a row inorg_toolsis modified. - Consider using the
org_tool_billing_state_set_updated_at()function if applicable.
| @@ -1,7 +1,7 @@ | |||
| \restrict dbmate | |||
|
|
|||
| -- Dumped from database version 15.17 (Debian 15.17-1.pgdg13+1) | |||
There was a problem hiding this comment.
Severity: warning
There is a PostgreSQL version bump from 15.17 to 16.14. Please ensure compatibility with existing applications and queries.
Suggestions:
- Verify compatibility of all application components and dependencies with PostgreSQL 16.14.
- Ensure all environments (dev, staging, prod) are aligned or tested for this version.
| -- Name: org_tool_billing_state_set_updated_at(); Type: FUNCTION; Schema: public; Owner: - | ||
| -- | ||
|
|
||
| CREATE FUNCTION public.org_tool_billing_state_set_updated_at() RETURNS trigger |
There was a problem hiding this comment.
Severity: warning
The function org_tool_billing_state_set_updated_at() is defined but appears to be unused.
Suggestions:
- Either add an
ON UPDATEtrigger to a relevant table (e.g.,org_tools) to use this function, or remove the function if it's not needed. - If intended for a future table, add a comment indicating its purpose and expected usage.
|
|
||
| 1. Load the diff from `SELECT diff FROM reviews WHERE id = $1 AND org_id = $2`. If the review has no diff, log and return without error (nothing to analyse). | ||
| 2. POST the diff as the Lambda payload to the tool's `lambda_arn` via HTTPS. | ||
| 3. On non-2xx response: return an error so River applies its standard retry policy. |
There was a problem hiding this comment.
Severity: warning
River retries requests on non-2xx responses. Ensure that a maximum retry limit is configured to prevent infinite retries for permanent errors.
Suggestions:
- Implement a maximum retry limit for River jobs to prevent indefinite retries on non-transient Lambda errors.
| In `WebhookOrchestratorV2` (or the unified processor), after diff extraction completes: | ||
|
|
||
| ```go | ||
| enabledTools, err := store.GetEnabledToolsForOrg(ctx, orgID) |
There was a problem hiding this comment.
Severity: critical
For the fan-out trigger, consider performing credit checks before inserting jobs. This would prevent queuing jobs for organizations that have insufficient credits or would exceed their budget.
Suggestions:
- Implement a credit budget check before inserting
ToolInvocationJobArgsinto River to prevent unnecessary job execution and potential cost overruns for organizations.
| In `WebhookOrchestratorV2` (or the unified processor), after diff extraction completes: | ||
|
|
||
| ```go | ||
| enabledTools, err := store.GetEnabledToolsForOrg(ctx, orgID) |
There was a problem hiding this comment.
Severity: warning
If GetEnabledToolsForOrg is slow or returns a large number of tools, this loop could become a performance bottleneck. Ensure its efficiency.
Suggestions:
- Optimize
GetEnabledToolsForOrgto ensure it's efficient, especially for organizations with many enabled tools, to avoid performance bottlenecks during job fan-out.
|
|
||
| result, err := decodeReviewResult(meta) | ||
| if err != nil { | ||
| return JSONErrorWithEnvelope(c, http.StatusInternalServerError, fmt.Sprintf("failed to decode review result: %v", err)) |
There was a problem hiding this comment.
Severity: warning
All decode errors are being swallowed, which can hide data corruption or invalid JSON.
Suggestions:
- Differentiate empty/missing metadata from actual JSON parsing errors.
- Log the decoding error to prevent silent failures.
| return nil, fmt.Errorf("failed to count batch IDs: %w", err) | ||
| } | ||
|
|
||
| sevCounts, err := s.repo.GetSeverityCounts(ctx, reviewID, orgID) |
There was a problem hiding this comment.
Severity: warning
The error from GetSeverityCounts is being swallowed, which can hide query or database failures.
Suggestions:
- Log the error to assist with debugging database or query issues.
| sevCounts = SeverityCounts{} | ||
| } | ||
|
|
||
| toolSum, err := s.repo.GetToolSummary(ctx, reviewID, orgID) |
There was a problem hiding this comment.
Severity: warning
The error from GetToolSummary is being swallowed, which can hide query or database failures.
Suggestions:
- Log the error to assist with debugging database or query issues.
| dbURL = os.Getenv("DATABASE_URL") | ||
| } | ||
| if dbURL == "" { | ||
| dbURL = "postgres://livereview:livereview_password_123@localhost:5432/livereview?sslmode=disable" |
There was a problem hiding this comment.
Severity: warning
A hardcoded fallback for the database URL is present. Consider using a dedicated test database setup or an in-memory database for better isolation during testing.
Suggestions:
- Use a test-specific database configuration that doesn't rely on a default hardcoded value, especially one with a default password.
- For unit tests, consider using an in-memory database like SQLite for faster and more isolated execution.
| ReviewID: reviewID, | ||
| OrgID: orgID, | ||
| EventType: "tool_dispatch", | ||
| Data: []byte(`{"tool_id": 1, "tool_name": "ruff", "status": "pending"}`), |
There was a problem hiding this comment.
Severity: warning
Tool event data is hardcoded. It is recommended to use constants or test data structures for improved clarity.
Suggestions:
- Define test data for
tool_dispatchandtool_resultevents as Go structs or constants to improve readability and maintainability.
| func (s *Server) UpsertAvailableTool(c echo.Context) error { | ||
| var req UpsertToolRequest | ||
| if err := c.Bind(&req); err != nil { | ||
| return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request body"}) |
There was a problem hiding this comment.
Severity: warning
A generic error message is being returned. It would be beneficial to return a specific binding error for improved debugging.
Suggestions:
- Return
c.JSON(http.StatusBadRequest, map[string]string{"error": err.Error()})to provide more specific details about the binding failure.
| if err := c.Bind(&req); err != nil { | ||
| return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request body"}) | ||
| } | ||
| if req.Name == "" || req.LambdaARN == "" { |
There was a problem hiding this comment.
Severity: warning
Basic validation is performed. Consider using a regular expression for the LambdaARN format to ensure correctness.
Suggestions:
- Add more robust validation for
LambdaARNusing a regular expression to ensure it matches expected AWS ARN format. - Consider adding length limits to
NameandDescriptionfields.
| tools := make([]ToolRow, 0) | ||
| for rows.Next() { | ||
| var t ToolRow | ||
| if err := rows.Scan(&t.ID, &t.Name, &t.Description, &t.LambdaARN, &t.Multiplier, &t.UseCase); err != nil { |
There was a problem hiding this comment.
Severity: warning
Error handling within the loop can lead to the process stopping upon encountering the first scan error.
Suggestions:
- Consider logging the specific row data that caused the scan error for better debugging, if possible without exposing sensitive data.
| var req struct { | ||
| Enabled *bool `json:"enabled"` | ||
| } | ||
| if err := c.Bind(&req); err != nil { |
There was a problem hiding this comment.
Severity: warning
A generic error message is being returned. It would be beneficial to return a specific binding error for improved debugging.
Suggestions:
- Return
c.JSON(http.StatusBadRequest, map[string]string{"error": err.Error()})to provide more specific details about the binding failure.
| ` | ||
| rows, err := r.db.QueryContext(ctx, query, reviewID, orgID) | ||
| if err != nil { | ||
| return SeverityCounts{}, fmt.Errorf("failed to query severity counts: %w", err) |
There was a problem hiding this comment.
Severity: warning
The error message 'failed to query severity counts: %w' is logged. Consider adding reviewID or orgID to the log for better context.
Suggestions:
- Include
reviewIDandorgIDin the error message for better debugging context.
| var lvl string | ||
| var cnt int | ||
| if err := rows.Scan(&lvl, &cnt); err != nil { | ||
| return SeverityCounts{}, err |
There was a problem hiding this comment.
Severity: critical
An unhandled scan error occurred. This could potentially lead to returning partial counts or corrupt data.
Suggestions:
- Log the specific error from
rows.Scanto understand why it failed. - Consider returning the error immediately or handling it more robustly to prevent partial or incorrect
SeverityCounts.
| ORDER BY ts ASC | ||
| ` | ||
| rows, err := r.db.QueryContext(ctx, query, reviewID, orgID) | ||
| if err != nil { |
There was a problem hiding this comment.
Severity: warning
The error message 'failed to query tool summary: %w' is logged. Consider adding reviewID or orgID to the log for better context.
Suggestions:
- Include
reviewIDandorgIDin the error message for better debugging context.
| for rows.Next() { | ||
| var ev rawToolEvent | ||
| if err := rows.Scan(&ev.EventType, &ev.Data, &ev.CreatedAt); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Severity: critical
An unhandled scan error occurred. This could potentially lead to returning partial tool events or corrupt data.
Suggestions:
- Log the specific error from
rows.Scanto understand why it failed. - Consider returning the error immediately or handling it more robustly to prevent partial or incorrect
toolEvents.
| for _, ev := range toolEvents { | ||
| if ev.EventType == "tool_dispatch" { | ||
| var d toolDispatchData | ||
| if err := json.Unmarshal(ev.Data, &d); err == nil && d.ToolName != "" { |
There was a problem hiding this comment.
Severity: warning
Tool dispatch events with unmarshal errors or empty tool names are silently skipped.
Suggestions:
- Log unmarshal errors for
tool_dispatchevents to identify malformed data. - Consider if
d.ToolName == ""should be an error or simply skipped.
| } | ||
| } else if ev.EventType == "tool_result" { | ||
| var res toolResultData | ||
| if err := json.Unmarshal(ev.Data, &res); err == nil && res.ToolName != "" { |
There was a problem hiding this comment.
Severity: warning
Tool result events with unmarshal errors or empty tool names are silently skipped.
Suggestions:
- Log unmarshal errors for
tool_resultevents to identify malformed data. - Consider if
res.ToolName == ""should be an error or simply skipped.
| } | ||
|
|
||
| // Fetch multipliers from available_tools table in database | ||
| multiplierMap := make(map[string]float64) |
There was a problem hiding this comment.
Severity: warning
Multiplier query errors are silently ignored, resulting in a default multiplier of 1.0 being used.
Suggestions:
- Log
tErrif fetching multipliers fails to understand why. - Consider if silently defaulting to
1.0is the desired behavior for cost calculation, or if it should be more explicit (e.g., return an error or setTotalCostUsdtonil).
| for tRows.Next() { | ||
| var tName string | ||
| var mult float64 | ||
| if err := tRows.Scan(&tName, &mult); err == nil { |
There was a problem hiding this comment.
Severity: warning
Multiplier scan errors are being silently ignored.
Suggestions:
- Log the
errfromtRows.Scanto identify issues withavailable_toolsdata.
| res, hasResult := resultsMap[toolName] | ||
| dispatchTime := dispatchedMap[toolName] | ||
|
|
||
| mult, exists := multiplierMap[strings.ToLower(toolName)] |
There was a problem hiding this comment.
Severity: warning
Cost calculation defaults to a multiplier of 1.0 if the multiplier is not found or is invalid.
Suggestions:
- Confirm if defaulting to
1.0forCreditsUsedandTotalCostUsdis the correct business logic when a multiplier is missing or invalid. This could lead to incorrect billing/accounting state. - Consider logging a warning when a multiplier is not found for a tool.
| item.Status = "clean" | ||
| } | ||
| } else { | ||
| if time.Since(dispatchTime) > 10*time.Second { |
There was a problem hiding this comment.
Severity: warning
A 10-second heuristic is used to differentiate between 'running' and 'pending' statuses, which may lead to inaccuracies.
Suggestions:
- This heuristic might misclassify tools if processing takes longer or events are delayed. Consider a more robust mechanism, like a dedicated 'tool_running' event or a configurable timeout.
- Document this heuristic clearly if it's intended behavior.
|
|
||
| sevCounts, err := s.repo.GetSeverityCounts(ctx, reviewID, orgID) | ||
| if err != nil { | ||
| // Log or ignore non-critical severity error |
There was a problem hiding this comment.
Severity: warning
The error from GetSeverityCounts is being silently ignored. Logging is needed to capture these errors.
Suggestions:
- Log the error from
s.repo.GetSeverityCountsto aid debugging, even ifsevCountsdefaults to empty.
| } | ||
|
|
||
| toolSum, err := s.repo.GetToolSummary(ctx, reviewID, orgID) | ||
| if err != nil { |
There was a problem hiding this comment.
Severity: warning
The error from GetToolSummary is being silently ignored. Logging is needed to capture these errors.
Suggestions:
- Log the error from
s.repo.GetToolSummaryto aid debugging, even iftoolSumdefaults tonil.
| } | ||
|
|
||
| // UpsertOrgTool inserts or updates the enabling configuration of a tool for a specific organization. | ||
| func (s *ToolsStore) UpsertOrgTool(ctx context.Context, orgID, toolID int64, enabled bool) (OrgToolRow, error) { |
There was a problem hiding this comment.
Severity: warning
In UpsertOrgTool, the config_json is not updated when a conflict occurs.
Suggestions:
- If
config_jsonshould be updatable via this function, addconfig_json = EXCLUDED.config_jsonto theON CONFLICTclause. - If
config_jsonis intended to be static after initial insert, add a comment explaining this design choice.
|
|
||
| // UpsertOrgTool inserts or updates the enabling configuration of a tool for a specific organization. | ||
| func (s *ToolsStore) UpsertOrgTool(ctx context.Context, orgID, toolID int64, enabled bool) (OrgToolRow, error) { | ||
| // First check if the tool actually exists in available_tools |
There was a problem hiding this comment.
Severity: warning
A check for the existence of a tool appears redundant if tool_id is a foreign key.
Suggestions:
- If
org_tools.tool_idhas a foreign key constraint toavailable_tools.id, remove thisSELECT EXISTSquery. TheINSERTwill fail with an FK violation if the tool does not exist, which can be handled. - If no FK exists, consider adding one for data integrity.
| return OrgToolRow{}, fmt.Errorf("failed to check tool existence: %w", err) | ||
| } | ||
| if !exists { | ||
| return OrgToolRow{}, sql.ErrNoRows |
There was a problem hiding this comment.
Severity: info
The function is returning sql.ErrNoRows for a non-existent tool, which might not be the intended behavior.
Suggestions:
- Consider defining a custom error type (e.g.,
ErrToolNotFound) for better semantic clarity for callers, rather than reusingsql.ErrNoRowswhich typically implies no rows were returned from a query, not that an entity doesn't exist.
| // Backend always returns one row per known layer, even at all-zero — treat that as fallback trigger for demo mode. | ||
| export function hasNoReviewLayerData(layers: ReviewLayer[]): boolean { | ||
| return layers.length === 0 || layers.every((layer) => layer.reviews_run === 0); | ||
| return false; |
There was a problem hiding this comment.
Severity: critical
This function always returns false, which appears to break its original intended functionality.
Suggestions:
- Revert to original logic or rename function to reflect new behavior.
- If intent is to force demo mode, apply this logic at the call site, not within the utility function.
| const [reviewLayers, setReviewLayers] = useState<ReviewLayers | null>(null); | ||
| const [loading, setLoading] = useState(true); | ||
| const [reviewLayers, setReviewLayers] = useState<ReviewLayers | null>(MOCK_REVIEW_LAYERS_OBJECT); | ||
| const [loading, setLoading] = useState(false); |
There was a problem hiding this comment.
Severity: warning
The loading variable is initialized to false. This might prevent the loading state from being displayed correctly.
Suggestions:
- Initialize
loadingtotrueif a loading indicator is desired while data fetches. - Consider if showing mock data immediately is sufficient without a separate loading state.
4 participants
No description provided.