feat(scanning): audit GitHub Actions workflows across repositories#33
feat(scanning): audit GitHub Actions workflows across repositories#33nakberli841-bot wants to merge 25 commits into
Conversation
- Add V2 Flyway migration for workflow_scan table with FK to repo
- Add WorkflowScanEntity and WorkflowScanRepository
- Add RepoRepository
- Implement WorkflowParser to flag unpinned actions, broad permissions
and missing OIDC with 0-100 risk scoring
- Implement WorkflowScanService to fetch workflow YAMLs from GitHub
and persist scan results
- Add WorkflowScanJob scheduled worker to scan all repos hourly
- Expose GET /api/repos/{repoId}/workflow-scans endpoint
- Add unit tests for WorkflowParser
- Add integration tests for WorkflowScanRepository and WorkflowScanController
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workflow scan persistence, parsing, scheduled execution, and an API endpoint to read stored scans for a repository. It also updates persistence mappings, migrations, worker wiring, and test configuration to support the new flow. ChangesWorkflow scan feature
Sequence Diagram(s)sequenceDiagram
participant WorkflowScanJob
participant AccountRepository
participant WorkflowScanService
participant GitHubClient
participant WorkflowParser
participant WorkflowScanRepository
WorkflowScanJob->>AccountRepository: findAll()
loop each account repo
WorkflowScanJob->>WorkflowScanService: scanRepo(repo, installationId)
WorkflowScanService->>GitHubClient: fetch .github/workflows listing
WorkflowScanService->>WorkflowScanRepository: deleteByRepoId(repoId)
loop each .yml/.yaml file
WorkflowScanService->>GitHubClient: download raw YAML
WorkflowScanService->>WorkflowParser: parse(workflowPath, yamlContent)
WorkflowScanService->>WorkflowScanRepository: save(WorkflowScanEntity)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
backend/libs/persistence/src/main/resources/db/migration/V2__create_workflow_scan_table.sql (1)
17-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a composite index aligned with the read query.
The primary read path
findByRepoIdOrderByScannedAtDescfilters onrepo_idand orders byscanned_at DESC. A composite(repo_id, scanned_at DESC)index lets Postgres satisfy both the filter and the sort without a separate sort step, which the currentrepo_id-only index does not.⚡ Suggested index
-CREATE INDEX idx_workflow_scan_repo_id ON workflow_scan(repo_id); +CREATE INDEX idx_workflow_scan_repo_id_scanned_at + ON workflow_scan(repo_id, scanned_at DESC);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/libs/persistence/src/main/resources/db/migration/V2__create_workflow_scan_table.sql` at line 17, The current workflow_scan index only covers repo_id, but the main read path in the repository query findByRepoIdOrderByScannedAtDesc also sorts by scanned_at DESC. Update the migration to use a composite index on workflow_scan that matches the filter and ordering, referencing the existing idx_workflow_scan_repo_id definition so Postgres can satisfy both without an extra sort.backend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.java (1)
54-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOrdering is not actually asserted.
savesAndFindsWorkflowScanByRepoIdpersists a single scan, sofindByRepoIdOrderByScannedAtDescwould pass regardless of sort direction. Consider inserting at least two scans with distinctscannedAtvalues and asserting the newest comes first to cover theOrderByScannedAtDesccontract that the API endpoint relies on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.java` around lines 54 - 82, The test in WorkflowScanRepositoryTest for savesAndFindsWorkflowScanByRepoId does not actually verify the OrderByScannedAtDesc behavior because it only persists one WorkflowScanEntity. Update this test to save at least two scans for the same RepoEntity with different scannedAt values, then assert that workflowScanRepository.findByRepoIdOrderByScannedAtDesc returns the newest WorkflowScanEntity first. Keep the existing repository method name and add assertions on the first and second results to cover the intended ordering contract.backend/libs/persistence/src/main/java/dev/cleat/persistence/repo/WorkflowScanRepository.java (1)
12-12: 🩺 Stability & Availability | 🔵 TrivialConsider a bulk delete here
deleteByRepoIdloads matchingWorkflowScanEntityrows and removes them one by one. A custom@Modifying@query("delete ...")would avoid the extra reads if this repo can grow large.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/libs/persistence/src/main/java/dev/cleat/persistence/repo/WorkflowScanRepository.java` at line 12, The deleteByRepoId method in WorkflowScanRepository currently relies on entity-by-entity removal, which can cause unnecessary reads for large repositories. Update this repository method to use a bulk delete approach with a custom `@Modifying` query so matching WorkflowScanEntity rows are deleted directly by repoId. Keep the method name as the entry point, but change the repository contract/annotations so the delete executes as a single bulk operation rather than loading entities first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/api/src/main/java/dev/cleat/api/CleatApiApplication.java`:
- Around line 7-8: The API application only enables repository scanning via
CleatApiApplication, so JPA entities under dev.cleat.persistence.entity are not
being discovered. Add entity scanning alongside the existing
`@EnableJpaRepositories` on CleatApiApplication, using `@EntityScan` with the
dev.cleat.persistence base package (matching CleatWorkerApplication) or
equivalent JPA entity scan configuration.
In
`@backend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.java`:
- Around line 12-14: The worker application scan setup is missing the
github-client module, so `WorkflowScanService` cannot resolve
`dev.cleat.githubclient.service.GitHubClient` at startup. Update
`CleatWorkerApplication` so its component scanning includes the
`dev.cleat.githubclient` package (or imports the github-client configuration
explicitly), alongside the existing `dev.cleat.persistence` and
`dev.cleat.scanning` setup, to ensure `WorkflowScanJob` can wire its
dependencies.
In `@backend/apps/worker/src/main/java/dev/cleat/worker/WorkflowScanJob.java`:
- Around line 25-38: `WorkflowScanJob.run()` is iterating detached
`AccountEntity`/`RepoEntity` instances, so the lazy `account.getRepos()` and
later `repo.getAccount().getLogin()` access can fail outside an active
persistence context. Fix this by loading the account/repo graph eagerly in
`accountRepository.findAll()` (for example with a `JOIN FETCH` or
`@EntityGraph`) or by moving the entire scan loop in `run()` into a
transactional boundary so `workflowScanService.scanRepo(...)` receives managed
entities.
In `@backend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowParser.java`:
- Around line 36-37: The permissions analysis in WorkflowParser has two coverage
gaps: job-level broad permission presets are not detected, and top-level scalar
write-all/read-all is treated as missing OIDC. Update the logic around the
permissions parsing and broadPermissions/missingOidc computation so both the
top-level permissions block and each job’s permissions are checked for
write-all/read-all, and treat write-all as implying id-token: write when setting
missingOidc. Use the existing WorkflowParser permissions handling and the
job-level inspection code to centralize this detection so the same rules apply
consistently.
- Around line 20-21: The YAML parsing in WorkflowParser currently uses the
default Yaml constructor and load path, which is not safe for untrusted workflow
files. Update WorkflowParser’s YAML creation to use SnakeYAML’s safe constructor
or safe loader configuration, and keep the parsing logic in the same place where
yaml.load(yamlContent) is called so external repository content is handled with
the restricted type set.
In
`@backend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowScanService.java`:
- Around line 42-48: In WorkflowScanService, stale scan rows are left behind
because the empty/null files early return happens before
workflowScanRepository.deleteByRepoId. Move the deleteByRepoId(repo.getId())
call so it runs before the files empty check in the scan workflow path, ensuring
old scans are cleared even when no workflows remain; keep the existing logic in
findByRepoIdOrderByScannedAtDesc unaffected.
- Line 60: Handle absolute download_url values separately in
WorkflowScanService: GitHubClient.get currently treats every input as a path on
api.github.com, so the contents API download_url is being built incorrectly.
Update the call site around the yamlContent fetch to use a request path/URI flow
that preserves absolute URLs, or add a separate GitHubClient method for absolute
URLs and use that here while keeping the bearer token attachment intact.
In
`@backend/libs/scanning/src/test/java/dev/cleat/scanning/WorkflowParserTest.java`:
- Around line 77-82: The test name in WorkflowParserTest is misleading because
emptyYamlReturnsZeroScore asserts a risk score of 20, not 0. Rename the test
method to reflect the actual behavior, such as emptyYamlScoresOnlyMissingOidc,
and keep the existing assertions in parser.parse and the WorkflowAnalysis
riskScore check unchanged.
---
Nitpick comments:
In
`@backend/libs/persistence/src/main/java/dev/cleat/persistence/repo/WorkflowScanRepository.java`:
- Line 12: The deleteByRepoId method in WorkflowScanRepository currently relies
on entity-by-entity removal, which can cause unnecessary reads for large
repositories. Update this repository method to use a bulk delete approach with a
custom `@Modifying` query so matching WorkflowScanEntity rows are deleted directly
by repoId. Keep the method name as the entry point, but change the repository
contract/annotations so the delete executes as a single bulk operation rather
than loading entities first.
In
`@backend/libs/persistence/src/main/resources/db/migration/V2__create_workflow_scan_table.sql`:
- Line 17: The current workflow_scan index only covers repo_id, but the main
read path in the repository query findByRepoIdOrderByScannedAtDesc also sorts by
scanned_at DESC. Update the migration to use a composite index on workflow_scan
that matches the filter and ordering, referencing the existing
idx_workflow_scan_repo_id definition so Postgres can satisfy both without an
extra sort.
In
`@backend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.java`:
- Around line 54-82: The test in WorkflowScanRepositoryTest for
savesAndFindsWorkflowScanByRepoId does not actually verify the
OrderByScannedAtDesc behavior because it only persists one WorkflowScanEntity.
Update this test to save at least two scans for the same RepoEntity with
different scannedAt values, then assert that
workflowScanRepository.findByRepoIdOrderByScannedAtDesc returns the newest
WorkflowScanEntity first. Keep the existing repository method name and add
assertions on the first and second results to cover the intended ordering
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 915914f2-ad1f-4a0b-8769-0468385c27fa
📒 Files selected for processing (24)
backend/apps/api/src/main/java/dev/cleat/api/CleatApiApplication.javabackend/apps/api/src/main/java/dev/cleat/api/controller/WorkflowScanController.javabackend/apps/api/src/test/java/dev/cleat/api/AbstractIntegrationTest.javabackend/apps/api/src/test/java/dev/cleat/api/WorkflowScanControllerTest.javabackend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.javabackend/apps/worker/src/main/java/dev/cleat/worker/WorkflowScanJob.javabackend/apps/worker/src/main/resources/application.ymlbackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/AccountEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/RepoEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/WorkflowScanEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/enums/AccountType.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/enums/Plan.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/enums/Visibility.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/repo/AccountRepository.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/repo/RepoRepository.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/repo/WorkflowScanRepository.javabackend/libs/persistence/src/main/resources/db/migration/V2__create_workflow_scan_table.sqlbackend/libs/persistence/src/test/java/dev/cleat/persistence/AccountRepositoryTest.javabackend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.javabackend/libs/scanning/build.gradle.ktsbackend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowAnalysis.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowParser.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowScanService.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/WorkflowParserTest.java
- Added installationId field (String) to AccountEntity - Updated V1 migration script to include installation_id column - Fixed compilation and consistency issues across modules
- Fix JUnit Platform launcher dependency in :libs:scanning - Resolve NoUniqueBeanDefinitionException in :apps:worker test context - Update API integration tests to match updated AccountEntity structure
…UniqueBeanDefinitionException
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/worker/src/test/resources/application.yml`:
- Around line 5-7: Disable scheduling in the worker test configuration so
`WorkflowScanJob` does not run during test startup. Update the test
`application.yml` used by `CleatWorkerApplication` to set
`spring.task.scheduling.enabled` to false while keeping the existing
`cleat.workflow-scan.interval-ms` settings as-is. This change should be made in
the test resource config that defines the worker application properties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e6f105d-544c-43f1-9716-bfe4d2ac22e6
📒 Files selected for processing (18)
backend/apps/api/src/main/java/dev/cleat/api/controller/WorkflowScanController.javabackend/apps/api/src/test/java/dev/cleat/api/WorkflowScanControllerTest.javabackend/apps/api/src/test/resources/application.ymlbackend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.javabackend/apps/worker/src/main/java/dev/cleat/worker/WorkflowScanJob.javabackend/apps/worker/src/test/java/dev/cleat/worker/AbstractIntegrationTest.javabackend/apps/worker/src/test/resources/application.ymlbackend/libs/github-client/src/main/java/dev/cleat/githubclient/config/GitHubConfig.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/AccountEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/entity/RepoEntity.javabackend/libs/persistence/src/main/java/dev/cleat/persistence/repository/WorkflowScanRepository.javabackend/libs/persistence/src/main/resources/db/migration/V1__create_account_and_repo_tables.sqlbackend/libs/persistence/src/main/resources/db/migration/V3__update_repo_and_add_tables.sqlbackend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.javabackend/libs/scanning/build.gradle.ktsbackend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowParser.javabackend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowScanService.javabackend/libs/scanning/src/test/java/dev/cleat/scanning/WorkflowParserTest.java
💤 Files with no reviewable changes (1)
- backend/apps/worker/src/test/java/dev/cleat/worker/AbstractIntegrationTest.java
✅ Files skipped from review due to trivial changes (2)
- backend/libs/persistence/src/main/java/dev/cleat/persistence/repository/WorkflowScanRepository.java
- backend/libs/persistence/src/main/resources/db/migration/V3__update_repo_and_add_tables.sql
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/apps/api/src/main/java/dev/cleat/api/controller/WorkflowScanController.java
- backend/apps/api/src/test/java/dev/cleat/api/WorkflowScanControllerTest.java
- backend/libs/scanning/build.gradle.kts
- backend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.java
- backend/apps/worker/src/main/java/dev/cleat/worker/CleatWorkerApplication.java
- backend/apps/worker/src/main/java/dev/cleat/worker/WorkflowScanJob.java
- backend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowScanService.java
- backend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowParser.java
- backend/libs/scanning/src/test/java/dev/cleat/scanning/WorkflowParserTest.java
- backend/libs/persistence/src/main/java/dev/cleat/persistence/entity/RepoEntity.java
martian56
left a comment
martian56
left a comment
There was a problem hiding this comment.
I went through the whole diff and every CodeRabbit thread, and checked each one against the current code rather than trusting the resolved flags. Short version: the valid CodeRabbit points are still open. Nine threads, two resolved, seven still active, and six of those seven are real and unaddressed. Two of them are not minor, they are bugs that only surface when the scheduled job actually runs, which is exactly why CI stays green.
The foundation is good. The parser structure, the risk scoring, the entity and migration, the endpoint, and the parser unit tests are all reasonable. But the end to end scan path does not work yet, and that is the part the tests do not exercise.
The two that block:
-
The scheduled job will throw on a lazy load (WorkflowScanJob, inline). run() is not transactional, so accountRepository.findAll() hands back detached entities, and account.getRepos() then trips a LazyInitializationException before scanRepo is ever reached. repo.getAccount().getLogin() inside scanRepo has the same problem since the repo arrives detached. The parser tests and the controller test never go through this path, so it passes CI and fails the moment the hourly job fires. CodeRabbit flagged this as critical and it is right. Easiest fixes: make run() @transactional, or load accounts with their repos via @EntityGraph or a JOIN FETCH query, or pass repo ids into a transactional service method and reload inside.
-
The workflow content fetch hits the wrong host (WorkflowScanService, inline). download_url from the contents API is an absolute raw.githubusercontent.com URL, but GitHubClient.get builds the request with .uri(builder -> builder.path(uri).build()) against the https://api.github.com base, so the absolute URL gets treated as a path under api.github.com. So even once the lazy load is fixed, no YAML actually comes back. Either fetch the content from the contents API response directly (it returns a base64 content field you can decode, no second call needed), or give GitHubClient a way to GET an absolute URL with a plain WebClient. Also worth noting these two together mean the feature has not run successfully end to end yet, so please verify against the compose stack with a seeded account before the next round.
The valid smaller ones, also still open:
-
Permission detection has gaps (WorkflowParser, inline). A top-level scalar permissions: write-all sets broadPermissions but leaves missingOidc true, so it gets both the broad penalty and a false missing-OIDC penalty even though write-all includes id-token: write. Job-level write-all / read-all is never counted as broad either. While you are in there, the map heuristic on line 36 (anyMatch value == write and size > 3) is arbitrary and will miss real broad grants, that is worth rethinking, not just patching.
-
Stale scans are never cleared when a repo loses all its workflows. The empty-files early return happens before deleteByRepoId, so old rows linger and keep showing up in the latest-scan query. Move the delete above the empty check.
-
WorkflowParserTest.emptyYamlReturnsZeroScore asserts a score of 20, not zero. Rename it so the name matches what it checks, something like emptyYamlScoresOnlyMissingOidc.
-
The worker test config leaves scheduling on, so WorkflowScanJob can fire during the integration test context. Add spring.task.scheduling.enabled: false to the worker test application.yml to keep tests deterministic.
The one I would push back on:
- CodeRabbit calls new Yaml().load() a major security hole on the grounds that Spring Boot 3.5.3 ships SnakeYAML 1.33. That premise is wrong. Boot 3.5.3 manages SnakeYAML 2.x, and 2.x is safe by default: the default loader rejects global tags, so arbitrary class instantiation from untrusted YAML is already blocked. So this is not a blocker and not for the reason given. I would still do two small things as plain hygiene: construct with a SafeConstructor so the intent is explicit and you are protected if the version ever moves, and wrap load() so a malformed workflow file throws something you handle rather than bubbling up. Neither is required to merge.
So to answer the actual question: no, the valid CodeRabbit comments are not fixed. Items 1 through 4, 6 and 7 all still stand in the code. Get the two runtime bugs sorted and the four cleanups in, and this is close. Happy to look again right after.
…or WorkflowScanJob
- replace arbitrary map heuristic with explicit PermissionStatus model - fix write-all scalar permission to correctly grant id-token: write - extend broad permission and OIDC checks to include job-level scopes
… local test compatibility
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/libs/persistence/src/test/resources/application.yml (1)
1-12: 🗄️ Data Integrity & Integration | 🔵 TrivialAdd one PostgreSQL-backed migration test in CI
Backend CI currently runs this module on H2 only, and
MODE=PostgreSQLwon’t catch all Flyway/dialect differences. Keep at least one job against a real PostgreSQL instance so schema drift is caught before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/libs/persistence/src/test/resources/application.yml` around lines 1 - 12, The test configuration currently relies on H2 with PostgreSQL mode, which does not exercise real Flyway or dialect behavior. Update the CI/test setup around application.yml and the persistence migration test path to run at least one job against a real PostgreSQL instance, using the existing Flyway-backed test configuration and relevant persistence test suite so schema drift is caught with the actual database engine.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/libs/persistence/src/test/resources/application.yml`:
- Around line 1-12: The test configuration currently relies on H2 with
PostgreSQL mode, which does not exercise real Flyway or dialect behavior. Update
the CI/test setup around application.yml and the persistence migration test path
to run at least one job against a real PostgreSQL instance, using the existing
Flyway-backed test configuration and relevant persistence test suite so schema
drift is caught with the actual database engine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e4d228f2-e838-4543-b7bb-5b273fc3ab2e
📒 Files selected for processing (11)
backend/apps/api/build.gradle.ktsbackend/apps/api/src/test/java/dev/cleat/api/AbstractIntegrationTest.javabackend/apps/api/src/test/resources/application.ymlbackend/apps/worker/build.gradle.ktsbackend/apps/worker/src/test/java/dev/cleat/worker/AbstractIntegrationTest.javabackend/apps/worker/src/test/resources/application.ymlbackend/libs/persistence/build.gradle.ktsbackend/libs/persistence/src/test/java/dev/cleat/persistence/AccountRepositoryTest.javabackend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.javabackend/libs/persistence/src/test/resources/application.ymlbackend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowParser.java
💤 Files with no reviewable changes (2)
- backend/libs/persistence/src/test/java/dev/cleat/persistence/WorkflowScanRepositoryTest.java
- backend/libs/persistence/src/test/java/dev/cleat/persistence/AccountRepositoryTest.java
✅ Files skipped from review due to trivial changes (1)
- backend/apps/worker/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/libs/scanning/src/main/java/dev/cleat/scanning/WorkflowParser.java
martian56
left a comment
martian56
left a comment
There was a problem hiding this comment.
Good progress, you cleared most of it. Four of the six are fixed and I verified each in the code:
- The lazy load is gone. run() is @transactional now, so findAll() and account.getRepos() are in the same session and it will not throw when the job fires.
- The stale-scan delete moved above the empty-files check, so a repo that loses all its workflows gets cleaned up.
- The permission audit is reworked properly. getPermissionStatus is applied at both top level and job level, write-all and read-all are flagged as broad in both places, and write-all no longer eats a false missing-OIDC penalty. That is the right shape.
- The test rename landed too.
One thing is still broken, and this one is on me, not you. I told you last round to read the base64 content field off the files the contents API already returned. That advice was wrong. The contents API only includes the content field when you GET a single file. For a directory listing like /contents/.github/workflows it returns an array where each entry has name, path, sha, and download_url, but no content. So file.get("content") is null for every entry, the loop logs "Content field missing" and skips them all, and nothing gets scanned. There is no WorkflowScanService test, so CI does not catch it. Sorry for sending you down that path. The correct options inline.
Two smaller things:
The scheduler is still enabled in the worker test config. WorkflowScanJob can fire while the test context is up. Add spring.task.scheduling.enabled: false to backend/apps/worker/src/test/resources/application.yml.
The switch from Testcontainers to H2 across the api, worker, and persistence tests is worth a second thought. I get the motivation, Docker in CI has been painful, but the migrations are written for Postgres and H2 in PostgreSQL mode only approximates it, so the persistence tests stop guaranteeing the schema actually works on the real database. The other two backend PRs in flight still use a real Postgres container, so this also splits the approach. Not a hard blocker, but please raise it with the others rather than quietly diverging, and at minimum keep the persistence-layer test on real Postgres since that is the one whose whole job is schema fidelity.
So: the content fetch is the real blocker and it is a quick fix once you take one of the two routes inline. Everything else here is solid. Ping me when the fetch is sorted and I will run through it again.
✅ Action performedComments resolved and changes approved. |
|
@nakberli841-bot Can you please resolve the conflicts |
|
@nakberli841-bot Hey there, are you still working on this? |
2 participants
What does this change?
Description
Adds automated auditing of GitHub Actions workflows across all connected repositories.
What was done
workflow_scantable via Flyway V2 migration with FK torepotableWorkflowScanEntityandWorkflowScanRepositoryto persist scan resultsRepoRepositoryfor repository-level DB accessWorkflowParserinlibs/scanningto parse workflow YAML files and flag:write-all/read-all)id-token: write)WorkflowScanServiceto fetch workflow YAMLs from GitHub API and persist resultsWorkflowScanJobinapps/workeras a scheduled job that scans all repos hourlyGET /api/repos/{repoId}/workflow-scansendpoint inapps/apiTests
WorkflowParserTest— unit tests covering unpinned actions, broad permissions, missing OIDC and clean workflow scenariosWorkflowScanRepositoryTest— integration tests with Testcontainers verifying DB read/write and delete operationsWorkflowScanControllerTest— full stack integration tests verifying endpoint response for populated and empty scan resultsRelated modules
libs/persistencelibs/scanningapps/workerapps/apiRelated issue
Closes # 10
Area
Screenshots
If this touches the UI, drop before and after screenshots here.
Checklist
bun run typecheck && bun run buildinapps/web(for frontend changes)Summary by CodeRabbit
GET /api/repos/{repoId}/workflow-scansto return the latest workflow scan results (risk score and detected risk signals).