Fix quality workflow enforcement#33
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (38)
📝 WalkthroughWalkthroughThis PR establishes agent-driven quality automation across the repository. It adds ChangesAgent Quality Automation & Security
Sequence DiagramsNot applicable for this update. The changes are infrastructure documentation, workflow automation, quality tooling setup, and targeted security hardening rather than new feature control flows. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive quality automation framework for AI agents, including new scripts, prompts, and updated repository guidelines in AGENTS.md. It also implements a security fix for prototype pollution in the URL tab state composable and updates the custom ESLint authorization rule to better handle Better Auth routes. Feedback focuses on the inappropriate exclusion of core directories from test coverage metrics, which undermines the newly established 80% threshold, and a breaking change in error handling caused by removing an export from a utility class. Additionally, several redundant markdown files used during the workflow setup should be removed to maintain repository cleanliness.
| 'extensions/**', | ||
| 'server/utils/**', | ||
| 'extensions/**/client/**', | ||
| 'extensions/**/extension.config.ts', | ||
| 'server/database/migrations/**', | ||
| 'server/plugins/**', | ||
| 'app/components/**', | ||
| 'app/composables/**', | ||
| 'app/types/**', |
There was a problem hiding this comment.
Excluding core directories such as app/components, app/composables, and server/utils from coverage reporting significantly undermines the quality gate. While the PR aims to enforce quality workflows, these exclusions make the 80% coverage threshold (defined in AGENTS.md) much easier to satisfy by ignoring the bulk of the application's logic. Unless there is a specific technical reason these cannot be tracked, they should remain included in coverage metrics to ensure meaningful quality enforcement.
There was a problem hiding this comment.
Partially applied. I agree the previous exclusion was too broad, so the gate now includes app utilities plus the changed URL tab composable instead of excluding all app composables. I did not include the full Vue component surface or all server/utils in the global threshold because a full inclusion run dropped the gate to lines 74.01%, functions 61.65%, branches 61.29%, largely from SFC render-function instrumentation and broad helper surfaces that need dedicated tests. The current gate is narrower but passes at lines 88.27%, statements 87.13%, functions 82.25%, branches 81.22%, while the component smoke tests still run as tests without dominating the global coverage threshold.
| * Error thrown for failed browser fetch responses with parsed API payloads. | ||
| */ | ||
| export class FetchResponseError extends Error { | ||
| class AppFetchResponseError extends Error { |
There was a problem hiding this comment.
Removing the export from the error class prevents consumers of throwFetchResponseError from using instanceof to identify and handle this specific error type. This is a breaking change for error handling logic. If the rename to AppFetchResponseError was intended to avoid naming collisions, the class should still be exported so it remains part of the usable API.
| class AppFetchResponseError extends Error { | |
| export class AppFetchResponseError extends Error { |
There was a problem hiding this comment.
Fixed. The app-side class is exported as AppFetchResponseError so consumers can still use instanceof without reintroducing the FetchResponseError auto-import collision.
| @@ -0,0 +1,389 @@ | |||
| # GCS-SSC Agent Operating Manual | |||
There was a problem hiding this comment.
The files in the gcs-ssc-agent-quality-workflows-md-files/ directory appear to be redundant as their content has already been merged into AGENTS.md or other prompt files. Keeping these intermediate or duplicate files in the repository increases maintenance overhead and can lead to confusion regarding the canonical source of truth. Consider removing this directory if the changes have been successfully integrated.
There was a problem hiding this comment.
Fixed. Removed the intermediate gcs-ssc-agent-quality-workflows-md-files directory from the PR; the canonical content remains in AGENTS.md and the prompt files.
omarmir
force-pushed
the
fix/code-quality-review
branch
2 times, most recently
from
7b1c354 to
d30d7b0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 @.gitignore:
- Around line 37-43: The current .gitignore negations (e.g.,
"!.agent/reports/.gitkeep", "!.agent/reports/pr/.gitkeep",
"!.agent/reports/whole/.gitkeep", "!.agent/findings/.gitkeep") won't take effect
unless the parent directories are unignored first; update the .gitignore to
explicitly unignore the parent directories (for example add negations like
"!.agent/", "!.agent/reports/", "!.agent/reports/pr/", "!.agent/reports/whole/",
"!.agent/findings/") before the .gitkeep negations so Git can track the nested
.gitkeep files referenced in the diff.
In `@extensions/gcs-gcforms-integration`:
- Line 1: The repository's submodule reference is pinned to a non-existent
commit SHA (8f3e5e50f7dd90ed8f2941271a005161751916c8) for the
GCS-SSC/gcs-gcforms-integration submodule; fix it by verifying the target commit
exists in that submodule repo and then updating the submodule reference to a
valid commit or branch/tag—edit the submodule entry (.gitmodules or the gitlink
in the commit) to point to a reachable SHA or switch to a branch/tag, run git
submodule sync and git submodule update --init --recursive (or use git submodule
set-branch / git update-index --cacheinfo if updating the gitlink), and commit
the corrected submodule reference so the repository no longer points to the
missing SHA.
In `@knip.json`:
- Around line 3-5: The knip.json currently sets a single explicit "entry" to
"scripts/admin-sql-dump.ts", which under-scopes Knip analysis and causes
excessive ignoreIssues; either remove the explicit "entry" key to let Knip
auto-detect entries, or expand "entry" to include the real application entry
points (e.g., add entries referencing app/, server/, extensions/, shared/ or the
Nuxt/server startup files) so analysis covers the whole project instead of only
scripts/admin-sql-dump.ts.
In `@package.json`:
- Around line 24-26: The npm scripts "quality:knip", "quality:deps", and
"quality:deps:graph" in package.json currently append "|| true", which masks
non‑zero exits and prevents failures from being captured by quality-whole.sh's
run_report() and its FAILED handling; remove the trailing "|| true" from these
three script definitions so knip and depcruise return their real exit codes
(keep the mkdir -p and output redirection as-is) allowing quality-whole.sh's
run_report() and the FAILED array logic to detect and propagate failures.
In `@packages/gcs-ssc-extensions`:
- Line 1: The pinned submodule commit 56df7f70d706b7173415a2b8c39fd1a5673aaa2c
for the GCS-SSC submodule does not exist in the remote
(https://github.com/GCS-SSC/gcs-ssc-extensions.git); fix this by either pushing
that missing commit to the remote repository or updating the submodule reference
to an existing commit hash (or to a branch/tag) and then update the submodule
entry (git submodule set-branch / git submodule update --init --recursive or
regenerate the .gitmodules entry) so the submodule pointer in the commit points
to a verifiable remote commit.
In `@scripts/agent/changed-files.sh`:
- Line 26: The git diff command in changed-files.sh omits deletions; update the
command that writes to "$REPORT_DIR/changed-files.txt" (the git diff ...
"$MERGE_BASE"...HEAD invocation) to include deleted files by adding the D flag
to the --diff-filter list (so deletions are captured) while keeping the rest of
the existing flags and output behavior intact.
In `@scripts/agent/quality-pr.sh`:
- Around line 14-25: The run_required function currently returns 1 on failure
which, under set -e, aborts the whole script before the summary is printed; edit
the run_required implementation (the function named run_required that appends to
PASSED and FAILED arrays) to remove the explicit "return 1" so failures are
recorded in FAILED but execution continues to run remaining checks, and ensure
the top-level exit behavior remains unchanged by relying on the final summary
logic that inspects FAILED and exits non‑zero after printing results; apply the
same removal of explicit returns in any similar required-check wrappers you find
in the script.
In `@scripts/agent/quality-whole.sh`:
- Around line 25-36: The run_gate function currently returns 1 on failure which,
combined with set -e, causes the whole script to exit immediately and prevents
the final summary block from running; edit the run_gate function (the one that
echoes "Running $label..." and manipulates PASSED and FAILED) to remove the
explicit `return 1` on failure so that failures are recorded in FAILED but
execution continues to the summary block, and let the final exit logic (which
inspects FAILED) determine the script exit code.
In `@vitest.config.ts`:
- Around line 38-40: The coverage exclusion contains a broad pattern
'extensions/**' that overrides the intended includes 'extensions/**/*.ts' and
'extensions/**/*.vue'; remove the 'extensions/**' entry from the
coverage.exclude array in vitest.config.ts (keep narrower exclusions like
'extensions/**/client/**' if needed) so the include patterns for extensions can
be applied and coverage thresholds enforced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80749b1b-8c69-414e-bbe6-11ed74ca0fd1
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.agent/README.md.agent/findings/.gitkeep.agent/prompts/incremental-pr-review.md.agent/prompts/whole-codebase-review.md.agent/reports/.gitkeep.github/codex/prompts/incremental-pr-review.md.github/codex/prompts/whole-codebase-review.md.github/pull_request_template.md.github/workflows/codex-pr-review.example.yml.github/workflows/quality-pr.yml.gitignoreAGENTS.mdapp/composables/useUrlTabState.tsapp/utils/fetch-error.tseslint-rules/require-authorize.jsextensions/gcs-automated-paymentsextensions/gcs-gcforms-integrationextensions/gcs-narrative-qualityextensions/gcs-narrative-tagsextensions/gcs-outcome-cost-allocationknip.jsonpackage.jsonpackages/gcs-ssc-extensionsscripts/agent/changed-files.shscripts/agent/optional-security-scan.shscripts/agent/quality-pr.shscripts/agent/quality-whole.shscripts/test-e2e.tsscripts/webcontainer.tsserver/api/extensions/[extensionKey]/[...route].tsserver/database/migrations/9999_seed.tstests/unit/extensions-sdk.test.tstests/unit/fetch-error.test.tstests/unit/require-authorize-rule.test.tstests/unit/use-url-tab-state.test.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- server/api/extensions/[extensionKey]/[...route].ts
| @@ -1 +1 @@ | |||
| Subproject commit f48eb1b76aa3f77253c825fa457184ff61a0afc4 | |||
| Subproject commit 8f3e5e50f7dd90ed8f2941271a005161751916c8 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the submodule commit is valid and from the expected branch
# Navigate to the submodule directory
cd extensions/gcs-gcforms-integration || exit 1
# Verify the commit exists
if git cat-file -e 8f3e5e50f7dd90ed8f2941271a005161751916c8 2>/dev/null; then
echo "✓ Commit exists in submodule"
# Show the commit details
echo -e "\nCommit details:"
git log -1 --oneline 8f3e5e50f7dd90ed8f2941271a005161751916c8
# Check which branches contain this commit
echo -e "\nBranches containing this commit:"
git branch -r --contains 8f3e5e50f7dd90ed8f2941271a005161751916c8 | head -10
else
echo "✗ Commit not found in submodule repository"
exit 1
fiRepository: GCS-SSC/gcs-ssc
Length of output: 102
🏁 Script executed:
# Check if the submodule directory exists and is properly configured
echo "=== Checking submodule directory ==="
ls -la extensions/gcs-gcforms-integration 2>&1 || echo "Directory does not exist"
echo -e "\n=== Checking .gitmodules ==="
cat .gitmodules 2>&1 | grep -A 5 "gcs-gcforms-integration" || echo ".gitmodules not found or entry not present"
echo -e "\n=== Checking git config ==="
git config --file .gitmodules --get-regexp "gcs-gcforms-integration" 2>&1
echo -e "\n=== Checking if submodule is initialized ==="
git config --get-regexp "submodule\..*\.url" 2>&1 | grep gcs-gcforms
echo -e "\n=== Checking git objects in parent repo ==="
git cat-file -e 8f3e5e50f7dd90ed8f2941271a005161751916c8 2>&1 && echo "✓ Commit exists in parent repo" || echo "✗ Commit not in parent repo"
echo -e "\n=== Checking git ls-files for submodule entry ==="
git ls-files --stage | grep gcs-gcforms-integrationRepository: GCS-SSC/gcs-ssc
Length of output: 1188
Submodule pin references non-existent commit.
The submodule is being pinned to commit 8f3e5e50f7dd90ed8f2941271a005161751916c8, but this commit does not exist in the submodule repository at https://github.com/GCS-SSC/gcs-gcforms-integration.git (error: "not our ref"). Verify the commit exists and is reachable in that repository, or use a valid commit reference.
🤖 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 `@extensions/gcs-gcforms-integration` at line 1, The repository's submodule
reference is pinned to a non-existent commit SHA
(8f3e5e50f7dd90ed8f2941271a005161751916c8) for the
GCS-SSC/gcs-gcforms-integration submodule; fix it by verifying the target commit
exists in that submodule repo and then updating the submodule reference to a
valid commit or branch/tag—edit the submodule entry (.gitmodules or the gitlink
in the commit) to point to a reachable SHA or switch to a branch/tag, run git
submodule sync and git submodule update --init --recursive (or use git submodule
set-branch / git update-index --cacheinfo if updating the gitlink), and commit
the corrected submodule reference so the repository no longer points to the
missing SHA.
| 'extensions/**', | ||
| 'server/utils/**', | ||
| 'extensions/**/client/**', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting coverage include/exclude patterns in vitest.config.ts..."
rg -n "coverage:|include:|exclude:|extensions/\\*\\*" vitest.config.ts
python - <<'PY'
from pathlib import Path
text = Path("vitest.config.ts").read_text()
print("\nSanity checks:")
print("include has extensions/**/*.ts :", "'extensions/**/*.ts'" in text)
print("include has extensions/**/*.vue :", "'extensions/**/*.vue'" in text)
print("exclude has extensions/** :", "'extensions/**'" in text)
print("\nExpected: if all three are true, extension coverage is likely excluded despite explicit include.")
PYRepository: GCS-SSC/gcs-ssc
Length of output: 798
Remove the broad extensions/** exclusion from coverage.
The pattern extensions/** in coverage.exclude directly contradicts extensions/**/*.ts and extensions/**/*.vue in coverage.include. Exclude patterns take precedence, so extension code remains hidden from coverage thresholds despite explicit inclusion, preventing proper enforcement of the 80%+ coverage requirement for feature work.
Suggested diff
exclude: [
'server/tests/**',
'tests/**',
'extensions/**/tests/**',
- 'extensions/**',
'server/utils/**',
'extensions/**/client/**',
'extensions/**/extension.config.ts',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'extensions/**', | |
| 'server/utils/**', | |
| 'extensions/**/client/**', | |
| 'server/utils/**', | |
| 'extensions/**/client/**', |
🤖 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 `@vitest.config.ts` around lines 38 - 40, The coverage exclusion contains a
broad pattern 'extensions/**' that overrides the intended includes
'extensions/**/*.ts' and 'extensions/**/*.vue'; remove the 'extensions/**' entry
from the coverage.exclude array in vitest.config.ts (keep narrower exclusions
like 'extensions/**/client/**' if needed) so the include patterns for extensions
can be applied and coverage thresholds enforced.
omarmir
force-pushed
the
fix/code-quality-review
branch
from
d30d7b0 to
b5ef024
Compare
1 participant
Summary
Review shape
mainValidation
bun run lintbun run typecheckbun run test:coverage- lines 88.34%, statements 87.18%, functions 82.04%, branches 81.40%PLAYWRIGHT_WORKERS=2 bun run scripts/test-e2e.ts- 80/80 passedbun run quality:security- 0 Semgrep findingsbun run quality:knip- 0 issuesNODE_OPTIONS=--max-old-space-size=8192 bun run quality:deps- 0 dependency violationsNODE_OPTIONS=--max-old-space-size=8192 bun run quality:deps:graphSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores