Feature/contract tests 258#295
Open
VIDYANKSHINI wants to merge 6 commits into
Open
Conversation
…, target normalization, and frontend races
There was a problem hiding this comment.
Pull request overview
This PR aims to add frontend-backend contract safety by (1) formalizing backend response schemas via Pydantic response_models and (2) tightening the frontend TypeScript client types, plus (3) adding a dynamic integration test that compares TS interfaces against the backend OpenAPI schema. It also introduces a new asset inventory/graph feature in both backend and frontend.
Changes:
- Added/expanded Pydantic response models and attached them to FastAPI routes to drive a stricter OpenAPI schema.
- Updated
frontend/src/api.tsto return strongly-typed promises for key API calls and added corresponding TS interfaces. - Introduced an asset inventory feature (DB tables + executor upsert/linking + new backend endpoints + new React Assets page), and added a backend integration test to detect contract drift.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
testing/backend/unit/test_assets.py |
Adds unit coverage for the new assets schema/relationships and REST endpoints. |
testing/backend/integration/test_api_contract.py |
Adds OpenAPI-vs-TypeScript contract drift checks. |
frontend/testing/unit/pages/Workflows.test.tsx |
Updates mocks for new workflow API response shapes (currently uses as any). |
frontend/testing/unit/pages/Reports.test.tsx |
Updates dashboard summary mocks (currently uses repeated as any). |
frontend/testing/unit/pages/Reports.preferredFormat.test.tsx |
Updates dashboard summary mock (currently uses as any). |
frontend/testing/unit/pages/Findings.test.tsx |
Extends API mocks to include getFindingDetails. |
frontend/testing/unit/AppRoutes.test.tsx |
Extends API mocks to include getFindingDetails. |
frontend/src/routes.ts |
Adds /assets route constant. |
frontend/src/pages/Findings.tsx |
Fetches and displays assets associated with a selected finding, with links into Assets page. |
frontend/src/pages/Assets.tsx |
New Assets inventory + topology graph UI with polling and query-param selection. |
frontend/src/components/Sidebar.tsx |
Adds Assets navigation item. |
frontend/src/App.tsx |
Wires the Assets route into the router. |
frontend/src/api.ts |
Adds many response interfaces and tightens return types for API functions. |
backend/secuscan/routes.py |
Attaches response models broadly; adds workflow schedule conversion helpers; implements assets endpoints and finding-assets association. |
backend/secuscan/models.py |
Adds/updates Pydantic models used as response_models (tasks, findings, workflows, assets, etc.). |
backend/secuscan/main.py |
Adds response_model to health endpoint. |
backend/secuscan/executor.py |
Updates asset inventory linkage after findings/reports are persisted. |
backend/secuscan/database.py |
Enables SQLite foreign keys and adds assets/link tables to schema. |
Comments suppressed due to low confidence (1)
backend/secuscan/routes.py:532
response_model=TaskResultwill cause FastAPI to filter out keys that this handler currently returns but the Pydantic model does not define (e.g.raw_output,command_used,preset,inputs). This is a breaking change for existing clients (the frontend TaskDetails page readsraw_output/command_used). Either extendTaskResultto include the returned fields (and update TS types accordingly), or stop returning those fields and update the frontend to match.
@router.get("/task/{task_id}/result", response_model=TaskResult)
async def get_task_result(task_id: str):
"""Get task execution result"""
db = await get_db()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+80
to
+84
| def cron_to_seconds(cron_str: str) -> int: | ||
| cron_str = cron_str.strip() | ||
| if cron_str.isdigit(): | ||
| return int(cron_str) | ||
| if cron_str in CRON_TO_SECONDS: |
Comment on lines
+96
to
+103
| def check_type_match(ts_type: str, schema: dict, schemas: dict, path: str): | ||
| # Strip null/undefined union parts from TS type | ||
| parts = [t.strip() for t in ts_type.split("|")] | ||
| clean_parts = [p for p in parts if p not in ["null", "undefined"]] | ||
| if not clean_parts: | ||
| return | ||
| clean_ts_type = clean_parts[0] | ||
|
|
Comment on lines
+123
to
+127
| } | ||
| } | ||
| window.addEventListener('popstate', handleLocationChange) | ||
| const interval = setInterval(handleLocationChange, 500) | ||
| return () => { |
Comment on lines
+39
to
+43
| const severityConfig: Record<string, string> = { | ||
| critical: 'bg-rag-red text-black border-rag-red/30', | ||
| high: 'bg-rag-amber text-black border-rag-amber/30', | ||
| medium: 'bg-rag-blue text-black border-rag-blue/30', | ||
| low: 'bg-charcoal-dark text-silver-bright border border-silver-bright/15', |
| const [selectedAssetId, setSelectedAssetId] = useState<string | null>(null) | ||
| const [selectedAssetDetails, setSelectedAssetDetails] = useState<any>(null) | ||
| const [detailsLoading, setDetailsLoading] = useState(false) | ||
| const navigate = useNavigate() |
| beforeEach(() => { | ||
| vi.mocked(getReports).mockResolvedValue({ reports: [generatingReport] }) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary as any) |
| beforeEach(() => { | ||
| vi.mocked(getReports).mockResolvedValue({ reports: [failedReport] }) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary as any) |
| beforeEach(() => { | ||
| vi.mocked(getReports).mockResolvedValue({ reports: [readyReport, generatingReport] }) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary as any) |
| beforeEach(() => { | ||
| vi.mocked(getReports).mockResolvedValue({ reports: [readyReport, generatingReport, failedReport] }) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary as any) |
| beforeEach(() => { | ||
| vi.mocked(getReports).mockResolvedValue({ reports: [readyReport, generatingReport, failedReport] }) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary) | ||
| vi.mocked(getDashboardSummary).mockResolvedValue(emptySummary as any) |
- routes.py: cron_to_seconds now raises HTTPException(400) for unrecognized schedule_interval values instead of silently falling back to 3600, making misconfigured workflows detectable early. - Assets.tsx: remove unused severityConfig constant and unused navigate import; replace 500ms setInterval URL-polling hack with proper useSearchParams() from react-router-dom so location changes are handled reactively without CPU wakeups. - Reports.test.tsx: expand emptySummary fixture to include all fields required by DashboardSummaryResponse (medium_findings, low_findings, info_findings, last_scan_time, recent_findings, scan_activity, running_tasks, recent_tasks) and remove all 'as any' casts so the mock remains fully type-safe.
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
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.
Description
This PR implements frontend-backend API contract safety by defining strict TypeScript response and request models in the React frontend and verifying them dynamically against the backend FastAPI OpenAPI schema.
Key changes include:
/health,/assets,/findings,/workflows,/task/{id}/status, and/task/{id}/result) with corresponding Pydantic schemas.api.tsto return strongly-typed promises (Promise<AssetsResponse>,Promise<TaskResult>, etc.) instead of untyped/anyrepresentations.testing/backend/integration/test_api_contract.pywhich dynamically extracts the OpenAPI schema components, parses TypeScript declarations, resolves schema inheritance (extends) and unions (anyOf/oneOf), and asserts that types and nullability match exactly.Related Issues
Closes #258
Type of Change
How Has This Been Tested?
We performed the following verification checks:
pytest testing/backend/integration/test_api_contract.py. It succeeds cleanly, and was verified to fail upon introducing deliberate type mismatches.pytestto run all 337 tests (all passed).npm run typecheck(tsc compiles with zero errors).npm run test(all 151 unit tests passed).Checklist