Dev to main#2
Merged
Merged
Conversation
Review Summary by QodoComprehensive Test Refactoring, Security Enhancements, and CI/CD Improvements
WalkthroughsDescription **Test Infrastructure Refactoring and Security Enhancements**
• Comprehensive test refactoring across backend packages to use shared test fixtures and baseline
data directories, reducing test initialization overhead and improving performance
• Converted 15+ integration tests to unit tests by extracting testable functions and removing
database dependencies
• Added WebSocket origin validation for CORS security and SSH host key verification with proper
shell argument quoting
• Implemented shared test app fixtures with sync.Once pattern for efficient baseline reuse across
multiple test packages
• Enhanced route coverage testing with matrix validation for OpenAPI spec completeness
• Added authentication middleware to Docker and operation log stream routes with superuser
verification
• Extracted testable functions from core logic (certificate resolution, monitor status queries,
agent bindings, software runtime bindings)
• Updated OpenAPI specifications with new endpoints (/tunnel/setup/{token},
/api/monitor/servers/{id}/container-telemetry)
• Added gitleaks configuration and CodeQL security scanning workflow for automated secret and
vulnerability detection
• Fixed async test queries across multiple frontend test files for proper async rendering handling
• Updated Go dependencies to version 1.26.2 and enhanced Makefile with actionlint, improved CI/CD
targets, and better tooling support
• Comprehensive documentation updates for test infrastructure, CI/CD workflows, and branch
protection strategy
Diagramflowchart LR
A["Test Fixtures<br/>Baseline Data"] -->|"Shared Reuse"| B["Optimized Tests<br/>Faster Execution"]
C["Unit Test<br/>Extraction"] -->|"Remove DB<br/>Dependencies"| B
D["WebSocket<br/>Origin Validation"] -->|"CORS Security"| E["Enhanced<br/>Security"]
F["SSH Host Key<br/>Verification"] -->|"Shell Quoting"| E
G["Gitleaks<br/>Config"] -->|"Secret Detection"| E
H["CodeQL<br/>Workflow"] -->|"Vulnerability<br/>Scanning"| E
I["OpenAPI<br/>Spec Updates"] -->|"New Endpoints"| J["API<br/>Documentation"]
K["Auth Middleware<br/>Addition"] -->|"Route Protection"| J
B -->|"Faster CI/CD"| L["Improved<br/>Development"]
E -->|"Secure<br/>Codebase"| L
J -->|"Clear API<br/>Contract"| L
File Changes1. web/src/pages/deploy/actions/action-utils.test.ts
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+15
to
+85
| name: Fast Checks | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| GITLEAKS_REPORT_PATH: build/reports/gitleaks-report.json | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version-file: backend/go.mod | ||
| cache-dependency-path: backend/go.sum | ||
|
|
||
| - name: Prepare Go tool bin | ||
| run: | | ||
| mkdir -p "$RUNNER_TEMP/bin" | ||
| echo "$RUNNER_TEMP/bin" >> "$GITHUB_PATH" | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "20" | ||
| cache: npm | ||
| cache-dependency-path: web/package-lock.json | ||
|
|
||
| - name: Install frontend deps | ||
| run: cd web && npm ci | ||
|
|
||
| - name: Install fast-check tools | ||
| run: | | ||
| set -euo pipefail | ||
| GOBIN="$RUNNER_TEMP/bin" go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest | ||
| GOBIN="$RUNNER_TEMP/bin" go install golang.org/x/vuln/cmd/govulncheck@latest | ||
|
|
||
| GL_VERSION="8.24.2" | ||
| ARCH="$(uname -m)" | ||
| case "$ARCH" in | ||
| x86_64) GL_ARCH="x64" ;; | ||
| aarch64|arm64) GL_ARCH="arm64" ;; | ||
| *) echo "Unsupported architecture: $ARCH" >&2; exit 1 ;; | ||
| esac | ||
| curl -sSfL "https://github.com/gitleaks/gitleaks/releases/download/v${GL_VERSION}/gitleaks_${GL_VERSION}_linux_${GL_ARCH}.tar.gz" \ | ||
| | tar -xz -C "$RUNNER_TEMP/bin" gitleaks | ||
|
|
||
| - name: Run fast lint | ||
| run: make lint fast | ||
| env: | ||
| GOLANGCI_LINT_BIN: ${{ runner.temp }}/bin/golangci-lint | ||
|
|
||
| - name: Run fast backend tests | ||
| run: make test backend fast | ||
|
|
||
| - name: Run frontend tests | ||
| run: make test web | ||
|
|
||
| - name: Run fast security checks | ||
| run: make sec fast | ||
| env: | ||
| GOVULNCHECK_BIN: ${{ runner.temp }}/bin/govulncheck | ||
| GITLEAKS_BIN: ${{ runner.temp }}/bin/gitleaks | ||
| GITLEAKS_REPORT_PATH: ${{ env.GITLEAKS_REPORT_PATH }} | ||
|
|
||
| - name: Upload fast CI gitleaks report | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: dev-fast-gitleaks-${{ github.sha }} | ||
| path: ${{ env.GITLEAKS_REPORT_PATH }} | ||
| if-no-files-found: ignore No newline at end of file |
Code Review by Qodo
1. PocketBase named in OpenAPI
|
| } | ||
|
|
||
| func buildShellCommand(command string, args ...string) string { | ||
| parts := make([]string, 0, len(args)+1) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+64
to
+65
| - name: System Cron | ||
| description: "PocketBase scheduled tasks and cron management APIs." |
There was a problem hiding this comment.
1. pocketbase named in openapi 📘 Rule violation § Compliance
The updated OpenAPI docs introduce PocketBase (a specific product name) in public-facing tag descriptions instead of using generic technology terms. This conflicts with the documentation requirement to avoid vendor/product specificity unless it is essential (e.g., explicit migration/integration context).
Agent Prompt
## Issue description
OpenAPI tag descriptions added/modified in this PR reference `PocketBase`, a specific product name, rather than using generic terms (e.g., “scheduled tasks” / “cron management”).
## Issue Context
The compliance rule requires generic technology terms in public-facing docs unless specificity is essential for an explicit migration/integration guide.
## Fix Focus Areas
- backend/docs/openapi/ext-api.yaml[64-65]
- backend/docs/openapi/api.yaml[72-72]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
214
to
223
| func expireDueCertificates(app core.App) error { | ||
| now := time.Now().UTC().Format(time.RFC3339) | ||
| now := time.Now().UTC() | ||
| records, err := app.FindRecordsByFilter( | ||
| "certificates", | ||
| "status = 'active' && expires_at != '' && expires_at <= {:now}", | ||
| "status = 'active' && expires_at != ''", | ||
| "", | ||
| 200, | ||
| 0, | ||
| map[string]any{"now": now}, | ||
| nil, | ||
| ) |
There was a problem hiding this comment.
2. Expiry sweep misses certificates 🐞 Bug ≡ Correctness
expireDueCertificates now queries an arbitrary first 200 "active" certificates with non-empty expires_at (no expires_at <= now filter and no ordering) and only expires those returned, so due certificates may remain unprocessed indefinitely. This can leave actually-expired certificates stuck in active status, breaking certificate lifecycle enforcement.
Agent Prompt
### Issue description
`expireDueCertificates` fetches only the first 200 active certificates with a non-empty `expires_at`, without filtering for those actually due (`expires_at <= now`) and without ordering/pagination. This can prevent some due certificates from ever being marked expired.
### Issue Context
The sweep runs on a ticker and should reliably converge on expiring all due certificates. Relying on an unsorted, capped query over *all* active certs with `expires_at` set breaks that guarantee.
### Fix Focus Areas
- backend/domain/certs/hooks.go[214-235]
- backend/domain/certs/hooks.go[194-211]
### What to change
- Reintroduce a DB-level due filter (e.g. `expires_at <= {:now}`) using `now.Format(time.RFC3339)`.
- Add a deterministic sort (e.g. `expires_at` ascending).
- Page through results (offset += limit) until fewer than `limit` records are returned, expiring each due record.
- Keep `certificateShouldExpire` as a safety check (e.g., to handle parse failures) but don’t rely on it to compensate for an overly broad, capped query.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the project’s test/quality and security posture (beyond the PR description’s “fix test”), including stricter Makefile workflows, CI updates, additional unit tests/mocks, OpenAPI coverage validation, and hardening around SSH + WebSocket security.
Changes:
- Stabilize/accelerate tests: raise Vitest timeouts, convert flaky assertions to async
findBy*, add targeted mocks, and introduce shared PocketBase baseline fixtures in multiple backend test packages. - Tighten quality gates: expand
make test/lint/sec/checkbehavior (OpenAPI checks, E2E smoke in strict mode, actionlint/typecheck in lint), add dev fast CI workflow, and refresh docs around testing/branch protection. - Security hardening: SSH host key verification support (known_hosts + strict mode), WebSocket Origin validation, Docker ext routes restricted to superusers, plus gitleaks/trivy config integration.
Reviewed changes
Copilot reviewed 86 out of 89 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/vitest.config.ts | Increase Vitest timeout to reduce flaky web tests. |
| web/src/routes/_app/_auth/resources/-service-instances.test.tsx | Add SecretForm mock to stabilize UI tests. |
| web/src/routes/_app/_auth/resources/-servers.test.tsx | Add mocks + extend test timeouts to reduce flakiness in ServersPage tests. |
| web/src/pages/system/TunnelsPage.test.tsx | Replace sync queries with async findByText to avoid race conditions. |
| web/src/pages/system/PlatformStatusPage.test.tsx | Refactor mock setup + split into focused tests; add cleanup. |
| web/src/pages/system/MonitorOverview.test.tsx | Convert assertions to async finds for stability. |
| web/src/pages/overview/OverviewPage.test.tsx | Convert assertions to async finds for stability. |
| web/src/pages/deploy/CreateDeploymentPage.test.tsx | Mock OrchestrationSection to decouple CreateDeploymentPage tests. |
| web/src/pages/deploy/actions/action-utils.test.ts | New unit test for building action WebSocket URLs (including token query param). |
| web/src/pages/apps/AppDetailPage.test.tsx | Add section mocks + make assertions more robust/async. |
| web/src/components/monitor/MonitorTargetPanel.test.tsx | Mock TimeSeriesChart for deterministic rendering in tests. |
| tests/README.md | Document test strategy + backend PocketBase baseline fixture rationale/guardrails. |
| tests/e2e/README.md | Update E2E entrypoints and planned layering (make test e2e fast, etc.). |
| specs/implementation-artifacts/story1.6-security-scanning.md | Update security scanning acceptance criteria (sec vs artifact scan). |
| specs/implementation-artifacts/story1.2-makefile.md | Update docs to reflect make test now includes E2E smoke in strict mode. |
| Makefile | Major updates: new test/lint/sec/check flows, OpenAPI validation, E2E integration, artifact scan, install tools, richer logging. |
| build/reports/gitleaks-report.json | Added gitleaks output artifact (currently empty). |
| build/.env | Replace committed secret key with placeholder text. |
| backend/infra/persistence/test_fixture_test.go | Add baseline+shared PocketBase test fixture for persistence tests. |
| backend/infra/persistence/provider_account_repository_test.go | Switch tests to shared baseline fixture. |
| backend/infra/persistence/instance_repository_test.go | Switch tests to shared baseline fixture. |
| backend/infra/persistence/connector_repository_test.go | Switch tests to shared baseline fixture. |
| backend/infra/migrations/test_fixture_test.go | Add baseline+shared PocketBase fixture for migration tests. |
| backend/infra/migrations/migrations_test.go | Switch migration tests to baseline fixture (and isolate where needed). |
| backend/infra/docker/ssh.go | Add shell-quoting for SSH commands + known_hosts-based host key verification with env overrides. |
| backend/infra/docker/ssh_test.go | Unit tests for quoting and host key callback behavior. |
| backend/go.mod | Patch-level Go version bump + update x/* indirect deps. |
| backend/go.sum | Corresponding module sum updates. |
| backend/domain/worker/worker_test.go | Switch to worker baseline fixture. |
| backend/domain/worker/test_fixture_test.go | New worker test baseline fixture + state reset helper. |
| backend/domain/worker/software_delivery_test.go | Switch to worker baseline fixture. |
| backend/domain/worker/monitoring_checks_test.go | Switch to worker baseline fixture. |
| backend/domain/worker/lifecycle_operations.go | Remove stray blank line. |
| backend/domain/worker/lifecycle_operations_test.go | Switch to worker baseline fixture. |
| backend/domain/worker/appos_agent_bindings.go | Refactor to smaller pure helpers for base URL + env injection. |
| backend/domain/worker/appos_agent_bindings_test.go | Update tests to use new pure helper functions. |
| backend/domain/software/service/service_test.go | Minor test flow tweak (early return after fatal). |
| backend/domain/software/runtime_bindings.go | Extract pure helper for installer URL resolution. |
| backend/domain/software/runtime_bindings_test.go | Convert to pure-function style tests (no PocketBase app required). |
| backend/domain/routes/server.go | Add WebSocket Origin validation (replaces CheckOrigin: true). |
| backend/domain/routes/server_test.go | Add tests for WebSocket origin validation incl forwarded headers. |
| backend/domain/routes/routes.go | Adjust auth middleware placement: remove query-token auth from general /api. |
| backend/domain/routes/routes_coverage_test.go | Expand OpenAPI coverage checks + add matrix extSurface coverage validation. |
| backend/domain/routes/resources_test.go | Introduce baseline PocketBase fixture for route tests + seed superuser once. |
| backend/domain/routes/docker.go | Restrict /api/ext/docker/* routes to superusers. |
| backend/domain/routes/docker_test.go | Add tests asserting superuser-only access for docker ext routes. |
| backend/domain/routes/deploy.go | Move log stream endpoint under /api/actions/{id}/stream and bind ws token auth + superuser auth. |
| backend/domain/routes/deploy_test.go | Add tests ensuring query-token auth works only for websocket stream route. |
| backend/domain/routes/apps.go | Fix indentation in temp compose write path. |
| backend/domain/resource/connectors/test_fixture_test.go | Add baseline fixture for connector runtime tests. |
| backend/domain/resource/connectors/runtime_test.go | Use baseline fixture when creating test app. |
| backend/domain/monitor/status/store/test_fixture_test.go | Add baseline fixture for status store tests. |
| backend/domain/monitor/status/store/store_test.go | Reduce integration dependency; use pure helper for summary building + baseline fixture. |
| backend/domain/monitor/status/store/projection.go | Extract BuildResourceCheckSummary for testability. |
| backend/domain/monitor/status/query.go | Extract pure helpers to build overview/synthesize app status from records. |
| backend/domain/monitor/status/query_test.go | Switch to pure-record unit test for overview building at scale. |
| backend/domain/monitor/status/query_internal_test.go | Switch to pure-record unit test for app status synthesis. |
| backend/domain/monitor/signals/platform/test_fixture_test.go | Add baseline fixture for platform observer tests. |
| backend/domain/monitor/signals/platform/observer.go | Make process resource collection injectable for tests. |
| backend/domain/monitor/signals/platform/observer_test.go | Use baseline fixture + inject resource function. |
| backend/domain/monitor/signals/platform/model.go | Add injected resource function to observer model. |
| backend/domain/monitor/signals/checks/test_fixture_test.go | Add baseline fixture for monitor checks tests. |
| backend/domain/monitor/signals/checks/credential_sweep_test.go | Use baseline fixture for checks tests. |
| backend/domain/monitor/signals/agent/test_fixture_test.go | Add baseline fixture for agent signal tests. |
| backend/domain/monitor/signals/agent/agent_test.go | Use baseline fixture for agent tests. |
| backend/domain/lifecycle/projection/updater_test.go | Convert to pure record tests (no PocketBase app required). |
| backend/domain/lifecycle/orchestration/test_fixture_test.go | Add baseline fixture for orchestration tests. |
| backend/domain/lifecycle/orchestration/runner_test.go | Use baseline fixture for runner context creation. |
| backend/domain/config/sharedenv/test_fixture_test.go | Add baseline fixture for shared env tests. |
| backend/domain/config/sharedenv/query_test.go | Use baseline fixture; minor refactors. |
| backend/domain/certs/resolve.go | Extract injectable helper for testability (resolveCertificateWith). |
| backend/domain/certs/resolve_test.go | Convert to pure unit tests using injected fns (no PocketBase app required). |
| backend/domain/certs/hooks.go | Refactor expiry sweep to pure helpers and injection points. |
| backend/domain/certs/hooks_test.go | Convert validation tests to pure helper-based tests (no PocketBase app required). |
| backend/domain/certs/hooks_expiry_test.go | Convert expiry tests to pure helper-based tests (no PocketBase app required). |
| backend/docs/openapi/README.md | Document expanded openapi-check guarantees. |
| backend/docs/openapi/group-matrix.yaml | Update surfaces/descriptions + add cron logs/ext monitoring telemetry surfaces. |
| backend/docs/openapi/ext-api.yaml | Add/adjust tags and paths (cron logs, telemetry, tunnel setup). |
| backend/docs/openapi/api.yaml | Sync merged OpenAPI output with ext spec updates. |
| backend/cmd/openapi/merge.go | Remove trailing whitespace/newline. |
| backend/cmd/openapi/gen.go | Extend generator to capture root-router routes and tunnel setup paths. |
| backend/build/reports/gitleaks-report.json | Added gitleaks output artifact with redacted findings. |
| .gitleaks.toml | Add repo gitleaks configuration and allowlists for known fixtures/dev keys. |
| .github/workflows/dev-fast-ci.yml | Add non-main “fast checks” workflow. |
| .github/workflows/codeql.yml | Add CodeQL workflows for Go + JS/TS. |
| .github/workflows/_quality-gate.yml | Add OpenAPI job and wire it into E2E job dependencies; switch E2E command to make test e2e fast. |
| .github/pull_request_template.md | Update validation checklist to use make test e2e fast. |
| .github/branch-protection-guide.md | Expand branch protection guidance + document workflow roles and CodeQL. |
Files not reviewed (1)
- web/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
build/reports/gitleaks-report.json:2
- This looks like a generated CI artifact (gitleaks JSON report). Keeping it committed will make local runs/CI overwrite tracked files and can create noisy diffs. Consider removing it from the repo and adding this path to
.gitignore, relying on CI artifacts for the uploaded report instead.
Comment on lines
214
to
223
| func expireDueCertificates(app core.App) error { | ||
| now := time.Now().UTC().Format(time.RFC3339) | ||
| now := time.Now().UTC() | ||
| records, err := app.FindRecordsByFilter( | ||
| "certificates", | ||
| "status = 'active' && expires_at != '' && expires_at <= {:now}", | ||
| "status = 'active' && expires_at != ''", | ||
| "", | ||
| 200, | ||
| 0, | ||
| map[string]any{"now": now}, | ||
| nil, | ||
| ) |
Comment on lines
+140
to
+149
| @echo "Installing Node.js CLI tools..." | ||
| @# Qodo CLI is published on npm as @qodo/command (provides the `qodo` binary) | ||
| @if ! command -v qodo >/dev/null 2>&1; then \ | ||
| echo "→ qodo..."; \ | ||
| npm install -g @qodo/command; \ | ||
| else \ | ||
| echo "✓ qodo already installed"; \ | ||
| fi | ||
| @echo "✓ Node.js CLI tools installed" | ||
| @echo "" |
Comment on lines
+501
to
+504
| echo "✗ actionlint is required for strict lint mode."; \ | ||
| echo " Expected binary at $(DEFAULT_ACTIONLINT_BIN) or on PATH."; \ | ||
| echo " Install it with 'make install' or run 'make lint fast' for advisory fallback mode."; \ | ||
| exit 1; \ |
Comment on lines
540
to
571
| fmt: | ||
| @echo "Formatting code ($(QUALITY_MODE))..." | ||
| @if [ -f "backend/go.mod" ]; then \ | ||
| @set -e; advisory_failures=""; \ | ||
| if [ -f "backend/go.mod" ]; then \ | ||
| echo "→ gofmt..."; \ | ||
| find backend -name "*.go" -exec gofmt -w {} +; \ | ||
| log_file=$$(mktemp); \ | ||
| set +e; find backend -name "*.go" -exec gofmt -w {} + >"$$log_file" 2>&1; status=$$?; set -e; \ | ||
| cat "$$log_file"; \ | ||
| if [ "$(QUALITY_MODE)" = "fast" ]; then \ | ||
| if [ "$$status" -ne 0 ]; then advisory_failures="$$advisory_failures gofmt"; fi; \ | ||
| else \ | ||
| if [ "$$status" -ne 0 ]; then \ | ||
| echo "✗ Format failed at: gofmt"; \ | ||
| rm -f "$$log_file"; \ | ||
| exit $$status; \ | ||
| fi; \ | ||
| fi; \ | ||
| rm -f "$$log_file"; \ | ||
| fi | ||
| ifeq ($(QUALITY_MODE),fast) | ||
| @if [ -f "web/node_modules/.bin/prettier" ]; then \ | ||
| echo "→ prettier..."; \ | ||
| cd web && npx prettier --write "src/**/*.{ts,tsx,css,json}" 2>/dev/null || true; \ | ||
| log_file=$$(mktemp); \ | ||
| set +e; cd web && npx prettier --write "src/**/*.{ts,tsx,css,json}" >"$$log_file" 2>&1; status=$$?; set -e; \ | ||
| cat "$$log_file"; \ | ||
| if [ "$$status" -ne 0 ]; then advisory_failures="$$advisory_failures prettier"; fi; \ | ||
| rm -f "$$log_file"; \ | ||
| fi | ||
| @if [ -n "$$advisory_failures" ]; then \ | ||
| echo "⚠ Fast format completed with issues:"; \ | ||
| for item in $$advisory_failures; do echo " - $$item"; done; \ | ||
| fi |
Comment on lines
+1
to
+5
| [ | ||
| { | ||
| "RuleID": "private-key", | ||
| "Description": "Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.", | ||
| "StartLine": 72, |
Comment on lines
+2
to
5
| .PHONY: help install tidy build run test test-strict test-fast lint lint-strict lint-fast fmt fmt-strict fmt-fast check check-fast sec sec-strict sec-fast artifact-scan \ | ||
| backend web backend-targeted fast strict build-local latest dev \ | ||
| image start stop restart logs stats delete rm kill-port redo \ | ||
| openapi-gen openapi-merge openapi-check openapi-sync |
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.
Summary
Describe the change in 2-5 lines.
What Changed
fix test
Why
Explain the problem, requirement, or risk this PR addresses.
Validation
make lintmake test backendmake test webmake e2eAdditional validation notes:
Risk Review
Contract Review
AI Assistance
Notes on AI usage:
Reviewer Checklist