fix(ci): harden CI against rubber-stamping (PRs to develop, continue-on-error, codecov, root-config paths) - #163
Merged
Conversation
added 2 commits
July 27, 2026 15:49
…per-workspace CIs Wave 2 of CI remediation (issues #159, #161). - ci.yml, server-ci.yml, cli-ci.yml, desktop-ci.yml, droplet-ci.yml, e2e.yml, codeql.yml, osv-scanner.yml, editorconfig-ci.yml, open-code-review.yml: add 'develop' to pull_request.branches so PRs targeting develop fire CI. - cli-ci.yml, desktop-ci.yml, droplet-ci.yml: add pnpm-workspace.yaml, package.json, pnpm-lock.yaml to paths: filters so root-config changes trigger per-workspace CIs. Push triggers remain on rebuild only to avoid double-runs on merge.
Wave 3 of CI remediation (issues #160, #161). - ci.yml SonarQube Scan step: remove continue-on-error: true so real scanner failures (e.g. auth errors) fail the build. Branch protection added in #158 requires this check. - ci.yml pnpm audit: keep continue-on-error but add follow-up step that fails only on NEW (un-ignored) critical advisories. - rust-ci/action.yml cargo audit: same pattern — keep continue-on-error, add follow-up step that fails only on new advisories not in security/risk-register.yaml. - osv-scanner.yml: keep continue-on-error: true on scan-scheduled (would fail on any transitive CVE) and document the rationale inline. - .github/codecov.yml: new file with informational thresholds. Project default threshold 2%, patch target 80%, both informational: true pending coverage milestone. fail_ci_if_error remains false in 3 locations to avoid blocking on Codecov infrastructure issues.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCI configuration now enforces detection of new advisories, expands workflow triggers for ChangesCI hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RustCIAction
participant CargoAudit
participant RiskRegister
RustCIAction->>CargoAudit: Generate JSON audit results
RustCIAction->>RiskRegister: Read registered GHSA IDs
RustCIAction->>RustCIAction: Fail for newly discovered advisories
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Comment |
Reviewer's GuideHardens CI and branch protections by running workflows on the develop branch, tightening behavior around vulnerability and quality scanners, adding coverage policy via Codecov, and ensuring root config changes trigger the correct workspace pipelines. Sequence diagram for Rust cargo audit new advisory handling in CIsequenceDiagram
participant GitHubActions
participant RustCIAction
participant CargoAudit
participant NodeCheckRustAdvisories
participant RiskRegister
GitHubActions->>RustCIAction: run Audit_dependencies
RustCIAction->>CargoAudit: cargo audit --json
CargoAudit-->>RustCIAction: /tmp/cargo-audit.json (may include advisories)
GitHubActions->>RustCIAction: always run Check_for_new_Rust_advisories
RustCIAction->>NodeCheckRustAdvisories: node -e (read /tmp/cargo-audit.json)
NodeCheckRustAdvisories->>RiskRegister: fs.readFileSync(security/risk-register.yaml)
NodeCheckRustAdvisories-->>RustCIAction: exit 1 if any advisory.id not in risk-register
RustCIAction-->>GitHubActions: job fails only on NEW advisories
Sequence diagram for pnpm audit critical advisory handling in CIsequenceDiagram
participant GitHubActions
participant CIWorkflow
participant PnpmAudit
participant NodeCheckCritical
GitHubActions->>CIWorkflow: run pnpm_audit (continue-on-error)
CIWorkflow->>PnpmAudit: pnpm audit --audit-level=critical --ignore GHSA-mp2f-45pm-3cg9
PnpmAudit-->>CIWorkflow: results (non-blocking)
GitHubActions->>CIWorkflow: always run Check_for_new_critical_advisories
CIWorkflow->>PnpmAudit: pnpm audit --audit-level=critical --json
PnpmAudit-->>CIWorkflow: /tmp/audit.json
CIWorkflow->>NodeCheckCritical: node -e (read /tmp/audit.json)
NodeCheckCritical-->>CIWorkflow: exit 1 if critical advisories not in ignored list
CIWorkflow-->>GitHubActions: job fails only on NEW critical advisories
Flow diagram for develop branch triggering hardened CI workflowsflowchart TD
PRDevelop["Pull request to develop"]
PRDevelop --> CI["ci.yml (monorepo CI)"]
PRDevelop --> ServerCI["server-ci.yml"]
PRDevelop --> CliCI["cli-ci.yml"]
PRDevelop --> DesktopCI["desktop-ci.yml"]
PRDevelop --> DropletCI["droplet-ci.yml"]
PRDevelop --> E2E["e2e.yml"]
PRDevelop --> CodeQL["codeql.yml"]
PRDevelop --> OSV["osv-scanner.yml"]
PRDevelop --> EditorconfigCI["editorconfig-ci.yml"]
PRDevelop --> OpenCodeReview["open-code-review.yml"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
SonarCloud Analysis ✅No BLOCKER, CRITICAL, or MAJOR issues found. |
- .github/codecov.yml: replace deprecated 'flags:' block with modern 'flag_management: default_rules + individual_flags' schema. Old schema was silently ignored causing 'configuration not found' warning. - .github/workflows/ci.yml: gate Post coverage gaps to PR on env.CODECOV_TOKEN != '' so missing secret no longer hard-fails the job (FATAL: CODECOV_TOKEN is not set surfaced after un-rubber-stamping).
BillyOutlast
marked this pull request as ready for review
July 27, 2026 20:03
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new inline Node checks for pnpm and cargo audit assume
/tmp/*.jsonexists and is valid JSON; if the underlying audit commands fail before writing (e.g., tool not installed, network issues), these steps will start failing the job unexpectedly—consider adding an existence check/try-catch around reading/parsing the JSON to make the behavior explicitly guarded. - Both audit follow-up steps embed fairly complex Node one-liners directly in the workflow; extracting these into small versioned scripts or a reusable action would make them easier to read, test locally, and evolve without touching workflow YAML.
- The cargo audit follow-up currently only filters new advisories by GHSA IDs extracted via a regex on
security/risk-register.yaml; if that register format changes or other advisory ID types are added, this logic may silently misclassify issues—consider tightening how you derive the set of known advisories (e.g., explicit YAML fields or a dedicated list) to avoid format-coupling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new inline Node checks for pnpm and cargo audit assume `/tmp/*.json` exists and is valid JSON; if the underlying audit commands fail before writing (e.g., tool not installed, network issues), these steps will start failing the job unexpectedly—consider adding an existence check/try-catch around reading/parsing the JSON to make the behavior explicitly guarded.
- Both audit follow-up steps embed fairly complex Node one-liners directly in the workflow; extracting these into small versioned scripts or a reusable action would make them easier to read, test locally, and evolve without touching workflow YAML.
- The cargo audit follow-up currently only filters new advisories by GHSA IDs extracted via a regex on `security/risk-register.yaml`; if that register format changes or other advisory ID types are added, this logic may silently misclassify issues—consider tightening how you derive the set of known advisories (e.g., explicit YAML fields or a dedicated list) to avoid format-coupling.
## Individual Comments
### Comment 1
<location path=".github/actions/rust-ci/action.yml" line_range="144-152" />
<code_context>
+ run: |
+ node -e "
+ const fs = require('fs');
+ const data = JSON.parse(fs.readFileSync('/tmp/cargo-audit.json', 'utf8'));
+ const vulnerabilities = data.vulnerabilities ? data.vulnerabilities.list || [] : [];
+ const known = [];
+ try {
+ const register = fs.readFileSync('security/risk-register.yaml', 'utf8');
+ const matches = register.match(/GHSA-[a-z0-9-]+/g) || [];
+ matches.forEach(m => known.push(m));
+ } catch (e) {}
+ const new_vulns = vulnerabilities.filter(v => !known.includes(v.advisory.id));
+ new_vulns.forEach(v => console.error('NEW RUST VULN:', v.advisory.id, v.package.name, v.advisory.title));
+ process.exit(new_vulns.length > 0 ? 1 : 0);
</code_context>
<issue_to_address>
**issue:** Handle missing/invalid cargo-audit output more defensively to avoid unexpected JSON.parse failures.
If `cargo audit` fails before writing `/tmp/cargo-audit.json` or writes an empty/partial file, `fs.readFileSync`/`JSON.parse` will throw and fail the step even when there are no new advisories. To match the risk register handling and make this more robust, consider wrapping the JSON read/parse in a try/catch and treating failures as "no data" (exiting 0) so the step only fails when it actually detects new advisories, not when the tool is flaky.
</issue_to_address>
### Comment 2
<location path=".github/workflows/ci.yml" line_range="104-108" />
<code_context>
+ - name: Check for new critical advisories
+ if: always()
+ run: |
+ pnpm audit --audit-level=critical --json > /tmp/audit.json 2>/dev/null || true
+ node -e "
+ const a = require('/tmp/audit.json');
+ const ignored = ['GHSA-mp2f-45pm-3cg9'];
+ const advisories = a.advisories ? Object.values(a.advisories) : [];
+ const critical = advisories.filter(v => v.severity === 'critical' && !ignored.includes(v.github_advisory_id));
+ critical.forEach(v => console.error('NEW CRITICAL:', v.github_advisory_id, v.module_name, v.title));
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing/invalid pnpm audit JSON to avoid hard failures from require().
If `pnpm audit` fails or outputs nothing, `/tmp/audit.json` may be missing or empty, causing `require('/tmp/audit.json')` to throw and fail the step even when no advisories were actually found. Consider wrapping the load/parse in a try/catch (treating errors as “no advisories”) or checking file existence/size before requiring it, so transient audit issues don’t break the workflow.
</issue_to_address>
### Comment 3
<location path=".github/codecov.yml" line_range="18-20" />
<code_context>
+ default:
+ target: 80%
+ informational: true
+ flags:
+ individual:
+ - name: server
+ paths:
+ - server/
</code_context>
<issue_to_address>
**issue (bug_risk):** Codecov flags configuration format is likely incorrect and may be ignored.
Codecov expects `flags` to be a mapping of flag names, not a list of `individual` entries. For example:
```yaml
after_n_builds: 1
coverage:
status: ...
flags:
server:
paths:
- server/
```
The current
```yaml
flags:
individual:
- name: server
paths:
- server/
```
doesn’t match the documented schema and is likely ignored. Please switch to the map-based format so the `server` flag is actually applied and coverage is split correctly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
✅ OpenCodeReview: No comments generated. Looks good to me. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced Jul 27, 2026
SonarCloud Analysis ✅No BLOCKER, CRITICAL, or MAJOR issues found. |
…N handling Address code review feedback on PR #163. - scripts/check-new-vulns.cjs (new): shared helper that parses pnpm and cargo audit JSON, compares against security/risk-register.yaml, and exits 1 only on confirmed new (un-accepted) advisories. - Defensive: missing/empty/malformed audit JSON exits 0 with a warning so transient tool failures no longer break the workflow. - Risk register parsing uses explicit 'advisory:' field line-scanning (no regex), supports both GHSA-* and RUSTSEC-* IDs (was GHSA-only). - .github/workflows/ci.yml: replace inline Node with script invocation. Adds --ignored GHSA-mp2f-45pm-3cg9 to match existing pnpm audit --ignore. - .github/actions/rust-ci/action.yml: replace inline Node with script. fallow-ignore-next-line on parseArgs/main: small CLI dispatcher + entry point; cyclomatic branches come from the CLI parser and JSON extraction paths. Refactoring would harm readability vs the simple suppression. Both functions are independently testable (run the script locally to exercise them).
Owner
Author
|
Addressed review feedback in commit Comment 1 (rust-ci/action.yml — defensive JSON handling):
Comment 2 (ci.yml — defensive JSON handling):
Comment 3 (codecov.yml — flags schema):
Overall #2 (extract to script):
Overall #3 (tighten risk-register parsing):
Smoke tests run locally (8 cases):
Commit on branch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/actions/rust-ci/action.yml:
- Around line 147-152: Update the risk-register read in the vulnerability
filtering logic to use the repository-root path
`${GITHUB_WORKSPACE}/security/risk-register.yaml` instead of the
workspace-relative path. Replace the empty catch around `fs.readFileSync` with
explicit error reporting and termination when the register cannot be read, while
preserving the existing known-advisory matching behavior.
- Around line 149-153: The vulnerability filtering around `known` and
`new_vulns` only matches `advisory.id` against GHSA aliases, so RustSec
advisories remain unrecognized. Update the comparison to treat a vulnerability
as known when either `v.advisory.id` or any entry in `v.advisory.aliases`
matches the registered identifiers, preserving the existing register parsing and
reporting behavior.
- Around line 129-145: The advisory checks unconditionally parse audit output
while suppressing tool failures, producing opaque errors when JSON is missing,
empty, or invalid. In the “Check for new Rust advisories” step in
.github/actions/rust-ci/action.yml at lines 129-145, validate
/tmp/cargo-audit.json before JSON.parse and emit a clear audit-tool or registry
failure diagnostic; apply the same missing, empty, and invalid-output handling
in .github/workflows/ci.yml at lines 101-106 for /tmp/audit.json.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df6b47d1-6c29-4158-93a1-0520d3c9024b
📒 Files selected for processing (12)
.github/actions/rust-ci/action.yml.github/codecov.yml.github/workflows/ci.yml.github/workflows/cli-ci.yml.github/workflows/codeql.yml.github/workflows/desktop-ci.yml.github/workflows/droplet-ci.yml.github/workflows/e2e.yml.github/workflows/editorconfig-ci.yml.github/workflows/open-code-review.yml.github/workflows/osv-scanner.yml.github/workflows/server-ci.yml
…7/9614/9624/9635 - scripts/check-new-vulns.cjs: default register path now uses GITHUB_WORKSPACE (env) with cwd fallback. Fixes r3660539607 where rust-ci composite action sets working-directory to cli/, desktop/src-tauri/, etc. — process.cwd() would have missed the register and flagged every Rust advisory as new. Verified via local smoke test with synthetic GITHUB_WORKSPACE=repo root and working-directory=subdir. - .github/workflows/cli-ci.yml, desktop-ci.yml, droplet-ci.yml: remove pnpm-lock.yaml from path triggers. Lockfile changes are JS-only and don't affect Rust builds (resolves r3660539614, r3660539635). Keep pnpm-workspace.yaml + package.json because those define workspace boundaries that can affect cargo resolver behavior. codecov.yml: already addressed in 171c267 — flag_management schema is the correct modern v4 form. The reviewer's flags: map suggestion targets the deprecated schema. RUSTSEC format support: already addressed in 171c267 — script uses explicit 'advisory:' field line-scan, not GHSA-only regex. JSON parse hardening: already addressed in 171c267 — script handles missing/empty/malformed JSON gracefully.
…rank Per OpenCodeReview reviews 4791154356, 4791154664, 4791192875 (and the related failing CI checks on PR #163): - .github/actions/rust-ci/action.yml: use $GITHUB_WORKSPACE to locate scripts/check-new-vulns.cjs. The composite action is invoked with working-directory set to a sub-crate (cli/, desktop/src-tauri/, libraries/droplet/), where a relative 'scripts/check-new-vulns.cjs' would not exist. This is the root cause of the CLI CI / Desktop CI / Droplet CI failures on PR #163 (MODULES_NOT_FOUND). - .github/workflows/ci.yml: sonar-pr-comment now guards on 'needs.sonar.result == "success"', matching the existing guard on sonar-sync. Previously the job would still try to query the API for findings when the scan itself had failed, posting confusing empty comments. - scripts/check-new-vulns.cjs: --help now prints a dedicated HELP_TEXT constant instead of reading the first 25 lines of the source file (fragile to comment edits). - scripts/check-new-vulns.cjs: severityRank() now returns 4 (critical) for unrecognized or missing severity strings instead of -1. The previous behavior silently filtered out advisories with missing severity strings — a real false-negative risk.
BillyOutlast
commented
Jul 27, 2026
BillyOutlast
left a comment
BillyOutlast
left a comment
Owner
Author
There was a problem hiding this comment.
Resolved in commit e73c5380. The script is now invoked via "$GITHUB_WORKSPACE/scripts/check-new-vulns.cjs" so the relative-path failure mode (cli/scripts/check-new-vulns.cjs MODULE_NOT_FOUND) is eliminated. This was the root cause of the CLI/Desktop/Droplet CI failures on PR #163.
Also fixed:
- r4791154664 (SonarCloud cascade) — added
needs.sonar.result == 'success'guard tosonar-pr-commentjob so it doesn't post confusing empty comments when the scan fails. Branch protection already enforces SonarCloud Scan as required, so scan failures correctly block merge regardless. - r4791192875 #2 (
--helpfragility) — replacedfs.readFileSync(__filename).split('\n').slice(0, 25)with a dedicatedHELP_TEXTconstant. - r4791192875 #3 (severityRank false-negative) — unknown/missing severity now returns 4 (critical) instead of -1, so advisories with unrecognized severity strings are no longer silently filtered out.
r4791192875 #1 (fallow- typo) — already resolved in commit 579c8f66 thread r3660585066 with fallow-docs evidence.
Issue comment 5096237772 is a meta-summary, no actionable content."
|
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
This was referenced Jul 27, 2026
|
✅ Created PR with unit tests: #168 |
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
1 participant
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.
Closes #157 #158 #159 #160 #161 #162
Summary
Adversarial 4-member audit (skeptic / workflow-archaeologist / log-forensics / remediation-architect) of PR #155 surfaced 5 hardened findings. This PR remediates all 5.
Findings & Fixes
Risk
Rollback
gh api -X DELETE repos/BillyOutlast/drop/rulesets/19847872and PUT 19814537 with empty rulesgit revertthis commitProvenance
Adversarial 4-member hyperplan, 3 rounds (independent → cross-attack → defend/refine/concede). 5 hardened findings, 7 overturned, 3 downgraded.
Summary by Sourcery
Harden CI and quality gates for develop branch changes while keeping dependency and coverage checks actionable but non-disruptive.
Enhancements:
CI:
Summary by CodeRabbit
Security
Quality & Testing
CI/CD
developbranch across builds, tests, end-to-end checks, and code review workflows.