feat: add remaining PFC checks and lightweight PR builds BED-6802#2239
feat: add remaining PFC checks and lightweight PR builds BED-6802#2239superlinkx merged 22 commits intomainfrom
Conversation
|
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 a Code Generation Check workflow, replaces the CI build job, updates VSCode launch configs, and applies multiple St Bernard code changes: license walk pruning, Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Runner as Runner
participant Repo as Repository
participant SB as go tool stbernard
PR->>GH: open/sync PR to main or stage/**
GH->>Runner: trigger Code Generation Check
Runner->>Repo: checkout code
Runner->>Runner: setup Go (from go.mod), install Node v22, enable Corepack
Runner->>SB: run "deps"
Runner->>SB: run "modsync", "generate", "license"
SB-->>Runner: generated/modified files
Runner->>Repo: compare worktree (skip `cmd/ui/public/mockServiceWorker.js`)
alt uncommitted generated output detected
Runner->>PR: emit ::error:: instructing `just prepare-for-codereview`
Runner->>GH: fail job
else no changes
Runner->>GH: succeed job
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e223fee to
35cb59d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/code-generation-check.yml (1)
24-26: Addreopenedandready_for_reviewhere.As written, this check will not run when a draft PR is marked ready without a new push, and it will not rerun on reopened PRs until the next commit. That leaves gaps in the new required gate.
Suggested fix
types: - opened + - reopened + - ready_for_review - synchronize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/code-generation-check.yml around lines 24 - 26, The workflow's events list under the "types" key is missing reopened and ready_for_review, so update the GitHub Actions trigger in .github/workflows/code-generation-check.yml by adding "reopened" and "ready_for_review" to the types array (the block containing "types:" and the entries "- opened" and "- synchronize") so the job will also run when a PR is reopened or when a draft PR is marked ready for review; preserve the existing indentation and YAML list formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/code-generation-check.yml:
- Around line 50-52: Replace the "Install Yarn" step that runs "npm install
--global yarn" with a step that runs "corepack enable" so the workflow uses the
repository-pinned Yarn (from packageManager/.yarnrc.yml) instead of installing a
global Yarn version; update the step name and command in the job that currently
contains the Install Yarn block to execute "corepack enable" to ensure CI uses
Yarn 4.13.0.
In `@packages/go/stbernard/command/license/internal/cmd.go`:
- Around line 152-156: The walk currently returns nil for directory entries when
ignorePath is true, which lets filepath.Walk continue descending into
directories named in ignoreDir; change the logic so that when info.IsDir() &&
slices.Contains(ignoreDir, info.Name()) you always return filepath.SkipDir to
prune that subtree regardless of ignorePath. In other words, make the
directory-ignore branch (using info.IsDir(), slices.Contains(ignoreDir,
info.Name()), and filepath.SkipDir) unconditional for directories, and treat
ignorePath only for per-file skipping so whole-directory ignores stop traversal.
In `@packages/go/stbernard/git/git.go`:
- Around line 98-111: The CheckClean logic currently uses a git diff invocation
(diffIndexPlan / cmdrunner.Run) which only catches unstaged changes; change
detection to run git status --porcelain=v1 --untracked-files=all (via
cmdrunner.Run) to determine repository dirtiness, and keep the existing git diff
invocation only for printing/display if you still want the diff output; update
the call-site symbols (replace or add a new command plan variable instead of
diffIndexPlan, use cmdrunner.Run(context.TODO(), <statusPlan>) and inspect
result.StandardOutput for non-empty output to return false, while still invoking
the existing diff command (diffIndexPlan) to print the detailed diff when
desired.
---
Nitpick comments:
In @.github/workflows/code-generation-check.yml:
- Around line 24-26: The workflow's events list under the "types" key is missing
reopened and ready_for_review, so update the GitHub Actions trigger in
.github/workflows/code-generation-check.yml by adding "reopened" and
"ready_for_review" to the types array (the block containing "types:" and the
entries "- opened" and "- synchronize") so the job will also run when a PR is
reopened or when a draft PR is marked ready for review; preserve the existing
indentation and YAML list formatting.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d195623c-dde8-4689-8bab-4856f3d6ed33
📒 Files selected for processing (6)
.github/workflows/code-generation-check.yml.vscode/launch.jsonpackages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/show/show.gopackages/go/stbernard/git/git.gopackages/go/stbernard/workspace/workspace.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/go/stbernard/git/git.go (1)
98-111:⚠️ Potential issue | 🔴 Critical
CheckCleanstill misses staged and untracked files.
git diffonly reports unstaged tracked-file edits, so this returnsclean=truefor staged-only changes and for untracked generated files. Downstream,packages/go/stbernard/command/show/show.gotrusts this boolean, which means the new CI gate can incorrectly pass with a dirty worktree.Use
git status --porcelain=v1 --untracked-files=allfor the dirtiness check, and keepgit diffonly for human-readable output if you still want to print the patch.Suggested direction
- diffIndexPlan := cmdrunner.ExecutionPlan{ - Command: "git", - Args: []string{"--no-pager", "diff"}, - Path: cwd, - Env: env.Slice(), - SuppressErrors: true, - } - result, err := cmdrunner.Run(context.TODO(), diffIndexPlan) + statusPlan := cmdrunner.ExecutionPlan{ + Command: "git", + Args: []string{"status", "--porcelain=v1", "--untracked-files=all"}, + Path: cwd, + Env: env.Slice(), + } + statusResult, err := cmdrunner.Run(context.TODO(), statusPlan) if err != nil { - return false, fmt.Errorf("git diff: %w", err) + return false, fmt.Errorf("git status --porcelain: %w", err) } - if len(result.StandardOutput.Bytes()) > 0 { + if statusResult.StandardOutput.Len() > 0 { slog.Info("Repository is dirty") - fmt.Fprint(os.Stdout, result.StandardOutput.String()) + diffPlan := cmdrunner.ExecutionPlan{ + Command: "git", + Args: []string{"--no-pager", "diff"}, + Path: cwd, + Env: env.Slice(), + } + if diffResult, err := cmdrunner.Run(context.TODO(), diffPlan); err == nil { + fmt.Fprint(os.Stdout, diffResult.StandardOutput.String()) + } return false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/stbernard/git/git.go` around lines 98 - 111, CheckClean currently uses a git diff command (diffIndexPlan + cmdrunner.Run + result.StandardOutput) which only detects unstaged changes; replace or augment that check with a git status invocation using args ["--no-pager","status","--porcelain=v1","--untracked-files=all"] (call via cmdrunner.Run) and treat any non-empty result.StandardOutput as dirty (return false after printing it). Keep the existing git diff call only for optional human-readable patch output (run it separately if you still want to print the patch), and ensure you still respect SuppressErrors and wrap errors from cmdrunner.Run with fmt.Errorf consistent with the existing pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/go/stbernard/git/git.go`:
- Around line 98-111: CheckClean currently uses a git diff command
(diffIndexPlan + cmdrunner.Run + result.StandardOutput) which only detects
unstaged changes; replace or augment that check with a git status invocation
using args ["--no-pager","status","--porcelain=v1","--untracked-files=all"]
(call via cmdrunner.Run) and treat any non-empty result.StandardOutput as dirty
(return false after printing it). Keep the existing git diff call only for
optional human-readable patch output (run it separately if you still want to
print the patch), and ensure you still respect SuppressErrors and wrap errors
from cmdrunner.Run with fmt.Errorf consistent with the existing pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 12f67e84-9a3b-4b70-bf29-fcae1736534a
📒 Files selected for processing (6)
.github/workflows/code-generation-check.yml.vscode/launch.jsonpackages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/show/show.gopackages/go/stbernard/git/git.gopackages/go/stbernard/workspace/workspace.go
✅ Files skipped from review due to trivial changes (2)
- packages/go/stbernard/workspace/workspace.go
- .github/workflows/code-generation-check.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/stbernard/command/license/internal/cmd.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-33: The build-application job currently inherits broad default
token permissions and has no timeout; update the job (build-application) to
declare explicit least-privilege token scopes (add a permissions block — e.g.,
permissions: contents: read — and only add additional scopes such as packages:
read or id-token: write if that job actually needs them) and add a runtime cap
via timeout-minutes (for example 30) under the job definition; keep the
SB_VERSION env as-is and ensure any required extra permissions are narrowly
scoped to specific needs.
- Around line 51-53: The CI step currently runs "npm install --global yarn"
which bypasses the pinned packageManager and .yarn/releases; replace that step
to use Corepack so the repo's pinned Yarn version is respected (enable Corepack
and prepare/activate the pinned yarn@4.13.0 from packageManager), ensuring the
workflow uses the repository's .yarn/releases and deterministic Yarn 4.13.0
instead of installing the latest global yarn.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cd59f6ba-761c-433b-99b6-d756a8dfe705
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/code-generation-check.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/code-generation-check.yml
b66cdb9 to
07c1228
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.github/workflows/ci.yml (2)
29-33:⚠️ Potential issue | 🟠 MajorAdd explicit job permissions and a timeout to harden CI execution.
build-applicationcurrently inherits default token permissions and has no runtime cap.Suggested fix
build-application: name: Build BloodHound Application runs-on: ubuntu-latest + permissions: + contents: read + timeout-minutes: 30 env: SB_VERSION: v0.0.0-pr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 29 - 33, For the build-application job, add an explicit permissions block and a runtime cap to harden CI: declare a minimal permissions map (for example set contents: read and any other specific tokens your steps require) under the build-application job and add a timeout-minutes value (e.g. 60) to prevent runaway runs; update the job that currently defines name: Build BloodHound Application / env: SB_VERSION to include these two fields so the job no longer relies on inherited token permissions and has a bounded execution time.
51-53:⚠️ Potential issue | 🟠 MajorUse Corepack instead of global Yarn install for deterministic toolchain resolution.
Line 53 installs whatever Yarn npm serves globally, which can drift from the repository’s pinned package-manager version.
Suggested fix
- name: Install Yarn run: | - npm install --global yarn + corepack enable + corepack install + yarn --version#!/bin/bash set -euo pipefail # Verify packageManager pinning and Yarn-related repo config fd -HI '^package\.json$' --exec rg -n '"packageManager"\s*:' {} \; fd -HI '^\.yarnrc\.yml$' fd -HI '^yarn\.lock$' fd -HI '^\.yarn$'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 51 - 53, The "Install Yarn" GitHub Actions step currently runs "npm install --global yarn", which can install an unpinned Yarn; replace that step with a Corepack-based approach: enable Corepack and prepare/activate the repository's pinned Yarn version (use the packageManager field in package.json or the repo's yarn.lock/.yarnrc.yml) instead of the global npm install so the CI uses the deterministic, pinned package manager.packages/go/stbernard/command/license/internal/cmd.go (1)
151-156:⚠️ Potential issue | 🟠 MajorPrune ignored directory roots from
ignorePathswithfilepath.SkipDir.At Line 154, only non-directories are short-circuited. Directory entries in
ignorePathsare still walked recursively, which defeats subtree pruning for those explicit directory ignores.Suggested fix
- // Always prune ignored directories so filepath.Walk does not descend into them. - if info.IsDir() && slices.Contains(ignoreDir, info.Name()) { + // Always prune ignored directories so filepath.Walk does not descend into them. + if info.IsDir() && slices.Contains(ignoreDir, info.Name()) { return filepath.SkipDir + } else if info.IsDir() && slices.Contains(ignorePaths, relPath) { + return filepath.SkipDir } else if ignorePath && !info.IsDir() { // Shortcut out without skipping directory (support specific file ignores) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/stbernard/command/license/internal/cmd.go` around lines 151 - 156, The walker currently short-circuits only for non-directories when ignorePath is true, so explicit directory entries in your ignore list still get traversed; update the walker (the block using info.IsDir(), slices.Contains(ignoreDir, info.Name()), and the ignorePath check) so that when ignorePath is true and the current entry is a directory you return filepath.SkipDir (instead of falling through), and keep returning nil for ignored files—this ensures directories listed in ignorePath/ignoreDir are pruned from recursion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 29-33: For the build-application job, add an explicit permissions
block and a runtime cap to harden CI: declare a minimal permissions map (for
example set contents: read and any other specific tokens your steps require)
under the build-application job and add a timeout-minutes value (e.g. 60) to
prevent runaway runs; update the job that currently defines name: Build
BloodHound Application / env: SB_VERSION to include these two fields so the job
no longer relies on inherited token permissions and has a bounded execution
time.
- Around line 51-53: The "Install Yarn" GitHub Actions step currently runs "npm
install --global yarn", which can install an unpinned Yarn; replace that step
with a Corepack-based approach: enable Corepack and prepare/activate the
repository's pinned Yarn version (use the packageManager field in package.json
or the repo's yarn.lock/.yarnrc.yml) instead of the global npm install so the CI
uses the deterministic, pinned package manager.
In `@packages/go/stbernard/command/license/internal/cmd.go`:
- Around line 151-156: The walker currently short-circuits only for
non-directories when ignorePath is true, so explicit directory entries in your
ignore list still get traversed; update the walker (the block using
info.IsDir(), slices.Contains(ignoreDir, info.Name()), and the ignorePath check)
so that when ignorePath is true and the current entry is a directory you return
filepath.SkipDir (instead of falling through), and keep returning nil for
ignored files—this ensures directories listed in ignorePath/ignoreDir are pruned
from recursion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ce39f668-2a37-420b-a36d-057a521e34ef
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/code-generation-check.yml.gitignore.vscode/launch.jsonpackages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/show/show.gopackages/go/stbernard/git/git.gopackages/go/stbernard/workspace/workspace.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- packages/go/stbernard/workspace/workspace.go
- .github/workflows/code-generation-check.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/go/stbernard/git/git.go
- .vscode/launch.json
- packages/go/stbernard/command/show/show.go
computator
left a comment
There was a problem hiding this comment.
Not done reviewing, but some thoughts so far
kpowderly
left a comment
There was a problem hiding this comment.
I know there are a few comments but honestly, nothing alarming to me!
2bb512f to
1dc3400
Compare
This reverts commit a73793d.
Description
Adjusts our GitHub actions to run direct builds (without all the docker overhead that slows down PRs), as well as adds missing just prepare-for-codereview checks to CI to prevent accidental misses. Importantly code generation, license checking, module syncing, and dependency installs occur and then are checked for file drift, rejecting the PR if committed files change due to automated processes.
Motivation and Context
Resolves BED-6802
This will make PRs a bit faster to complete pipelines while also preventing code generation/missing license checks/other drift from merging into main
How Has This Been Tested?
Extensive attempts and review of actions during the PR. Everything reacts as expected in the happy and unhappy paths.
Types of changes
Checklist:
Summary by CodeRabbit
Chores
Bug Fixes
Improvements