feat(ci): add public content safeguards#1577
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (47)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (41)
📝 WalkthroughWalkthroughThe PR adds public-content scanning and metadata handling, threads those findings into quality-gate and semantic review, adds comment-audit and workflow wiring, and expands dry-run placeholder synthesis and output formatting. ChangesPublic content quality gate and review flow
Dry-run placeholder synthesis
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)scripts/semantic-review-publish.test.js[ ... [truncated 5517 characters] ... a Restricted Directory ('Path Traversal'). [REFERENCES]\n - https://cwe.mitre.org/data/definitions/22.html", 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@50db4677bed841c7361d159899a40105e72eedfd🧩 Skill updatenpx skills add larksuite/cli#feat/public-leak-detection-cli -y -g |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/qualitygate/rules/dryrun_test.go (1)
308-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
FlagDefault(DefValue) consumption is not actually exercised.
defaultPositiveInteger(ctx.FlagDefault, "20")returns the same value whether or notFlagDefaultis read here, because thepage-sizeDefValue("20") equals the hardcoded fallback. The other numeric tests use theparamsflag (empty default → fallback"1893456000"), so no test proves that a manifestDefValueis preferred over the fallback. Consider settingDefValueto a value distinct from the fallback (e.g."50") and asserting it propagates, so the newFlagDefaultpath has real coverage.💚 Example tweak to assert the DefValue path
- {Name: "page-size", TakesValue: true, Usage: "page size, 20-100 (default 20)", DefValue: "20"}, + {Name: "page-size", TakesValue: true, Usage: "page size, 20-100 (default 50)", DefValue: "50"},- cliBin, argsPath := fakeDryRunCLI(t, `{"api":[{"method":"GET","url":"/open-apis/vc/v1/bots/events","params":{"meeting_id":"400000000001","page_size":20}}]}`) + cliBin, argsPath := fakeDryRunCLI(t, `{"api":[{"method":"GET","url":"/open-apis/vc/v1/bots/events","params":{"meeting_id":"400000000001","page_size":50}}]}`)- wantArgs := []string{"vc", "+meeting-events", "--meeting-id", "400000000001", "--page-size", "20", "--dry-run"} + wantArgs := []string{"vc", "+meeting-events", "--meeting-id", "400000000001", "--page-size", "50", "--dry-run"}As per coding guidelines: "Every behavior change needs a test alongside the change."
Source: Coding guidelines
.github/workflows/ci.yml (1)
95-100: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseon the script-test checkout.
make script-testdoesn't perform authenticated git operations, so the persisted token (withfetch-depth: 0) is unnecessary surface flagged by zizmorartipacked.🔒 Proposed change
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/ci.yml around lines 95 - 100, The script-test checkout is persisting Git credentials unnecessarily. Update the actions/checkout step used before make script-test to set persist-credentials to false, while keeping the existing fetch-depth behavior, so the workflow no longer carries an unused token surface.Source: Linters/SAST tools
.github/workflows/comment-audit.yml (1)
18-21: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseon checkout.This audit job never performs authenticated git operations, so the persisted
GITHUB_TOKENin the runner git config is unnecessary attack surface (zizmorartipacked). Disable credential persistence.🔒 Proposed change
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + persist-credentials: false - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6🤖 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 @.github/workflows/comment-audit.yml around lines 18 - 21, The checkout step in the workflow should stop persisting the default GitHub token because this audit job does not need authenticated git access. Update the actions/checkout usage in the workflow to set persist-credentials to false while leaving the rest of the job intact, so the runner does not keep unnecessary credentials in its git config.Source: Linters/SAST tools
internal/qualitygate/publiccontent/collect.go (1)
333-341: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
CombinedOutputmixes stderr into parsed git output.Every git call here (including
diff/log/showwhose stdout is line-parsed for hunks and credential text) usesCombinedOutput, so git diagnostics written to stderr (e.g.warning: LF will be replaced by CRLF, advice hints) get interleaved into the data being scanned. This can break hunk/line parsing or surface spurious scanned text. Prefer capturing stdout viacmd.Output()and keep stderr only for the error message.♻️ Proposed change
func gitOutput(ctx context.Context, repo string, args ...string) ([]byte, error) { cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = repo - out, err := cmd.CombinedOutput() - if err != nil { - return nil, fmt.Errorf("git %s: %w\n%s", strings.Join(args, " "), err, out) - } - return out, nil + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return nil, fmt.Errorf("git %s: %w\n%s", strings.Join(args, " "), err, stderr.Bytes()) + } + return stdout.Bytes(), nil }🤖 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 `@internal/qualitygate/publiccontent/collect.go` around lines 333 - 341, The gitOutput helper currently uses CombinedOutput, which mixes stderr into the returned git data and can corrupt parsers that consume stdout from git diff/log/show. Update gitOutput to capture stdout only from exec.CommandContext via cmd.Output() while still preserving stderr in the returned error path, and keep the existing context/repo/args handling and formatting in gitOutput.
🤖 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 `@internal/qualitygate/cmd/comment-audit/main_test.go`:
- Around line 44-48: The unsafe-path validation in commentBody is returning a
generic fmt.Errorf-style wrapper and uses the wrong CLI flag in its error text,
so update commentBody to return the appropriate typed errs.* validation error
with --file metadata and adjust the test in main_test to assert it via
errs.ProblemOf instead of matching an error string substring. Make sure the flag
name in both the production path validation and the test expectation is
consistent, and keep the existing commentBody symbol as the place to fix the
validation/error construction.
In `@internal/qualitygate/publiccontent/collect_test.go`:
- Around line 653-658: The slice literal in the test setup uses inconsistent
indentation, so gofmt will reformat it and cause the pre-PR check to fail.
Update the indentation of the excluded []string literal in collect_test.go to
match gofmt’s standard spacing, keeping the list entries and closing brace
aligned as gofmt would in the test block.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 95-100: The script-test checkout is persisting Git credentials
unnecessarily. Update the actions/checkout step used before make script-test to
set persist-credentials to false, while keeping the existing fetch-depth
behavior, so the workflow no longer carries an unused token surface.
In @.github/workflows/comment-audit.yml:
- Around line 18-21: The checkout step in the workflow should stop persisting
the default GitHub token because this audit job does not need authenticated git
access. Update the actions/checkout usage in the workflow to set
persist-credentials to false while leaving the rest of the job intact, so the
runner does not keep unnecessary credentials in its git config.
In `@internal/qualitygate/publiccontent/collect.go`:
- Around line 333-341: The gitOutput helper currently uses CombinedOutput, which
mixes stderr into the returned git data and can corrupt parsers that consume
stdout from git diff/log/show. Update gitOutput to capture stdout only from
exec.CommandContext via cmd.Output() while still preserving stderr in the
returned error path, and keep the existing context/repo/args handling and
formatting in gitOutput.
🪄 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: 19783f07-946a-486c-9ea7-757464a2d3e5
📒 Files selected for processing (47)
.github/workflows/ci.yml.github/workflows/comment-audit.yml.github/workflows/semantic-review.ymlMakefileinternal/qualitygate/cmd/comment-audit/main.gointernal/qualitygate/cmd/comment-audit/main_test.gointernal/qualitygate/cmd/quality-gate/main.gointernal/qualitygate/cmd/quality-gate/main_test.gointernal/qualitygate/cmd/semantic-review/main.gointernal/qualitygate/cmd/semantic-review/main_test.gointernal/qualitygate/config/semantic/policy.jsoninternal/qualitygate/facts/schema.gointernal/qualitygate/facts/schema_test.gointernal/qualitygate/publiccontent/collect.gointernal/qualitygate/publiccontent/collect_test.gointernal/qualitygate/publiccontent/comment_audit.gointernal/qualitygate/publiccontent/comment_audit_test.gointernal/qualitygate/publiccontent/metadata.gointernal/qualitygate/publiccontent/metadata_test.gointernal/qualitygate/publiccontent/rules.gointernal/qualitygate/publiccontent/scan.gointernal/qualitygate/publiccontent/scan_test.gointernal/qualitygate/publiccontent/types.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/qualitygate/semantic/client.gointernal/qualitygate/semantic/gatekeeper.gointernal/qualitygate/semantic/gatekeeper_test.gointernal/qualitygate/semantic/prompt.gointernal/qualitygate/semantic/prompt_contract_test.gointernal/qualitygate/semantic/schema.gointernal/qualitygate/semantic/scope.gointernal/qualitygate/semantic/scope_test.gointernal/qualitygate/semantic/view.gointernal/qualitygate/semantic/view_test.gointernal/qualitygate/semantic/waiver.gointernal/qualitygate/semantic/waiver_test.goscripts/ci-quality-summary-publish.jsscripts/ci-quality-summary-publish.test.jsscripts/ci-workflow.test.shscripts/semantic-review-publish.jsscripts/semantic-review-publish.test.jsscripts/semantic-review-verify-artifact.jsscripts/semantic-review-verify-artifact.test.jsscripts/semantic-review-workflow.test.sh
5ba6b26 to
b5b71d9
Compare
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 @.github/workflows/ci.yml:
- Around line 96-99: Disable credential persistence for the script-test checkout
by updating the actions/checkout step in the CI workflow to include
persist-credentials: false, matching the hardened setup already used in lint;
locate the affected checkout step in the script-test job and add the setting
alongside fetch-depth to prevent credentials from being retained for later
steps.
🪄 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: 93d8266b-c71b-4c8f-aa16-b4dd3479bcb3
📒 Files selected for processing (47)
.github/workflows/ci.yml.github/workflows/comment-audit.yml.github/workflows/semantic-review.ymlMakefileinternal/qualitygate/cmd/comment-audit/main.gointernal/qualitygate/cmd/comment-audit/main_test.gointernal/qualitygate/cmd/quality-gate/main.gointernal/qualitygate/cmd/quality-gate/main_test.gointernal/qualitygate/cmd/semantic-review/main.gointernal/qualitygate/cmd/semantic-review/main_test.gointernal/qualitygate/config/semantic/policy.jsoninternal/qualitygate/facts/schema.gointernal/qualitygate/facts/schema_test.gointernal/qualitygate/publiccontent/collect.gointernal/qualitygate/publiccontent/collect_test.gointernal/qualitygate/publiccontent/comment_audit.gointernal/qualitygate/publiccontent/comment_audit_test.gointernal/qualitygate/publiccontent/metadata.gointernal/qualitygate/publiccontent/metadata_test.gointernal/qualitygate/publiccontent/rules.gointernal/qualitygate/publiccontent/scan.gointernal/qualitygate/publiccontent/scan_test.gointernal/qualitygate/publiccontent/types.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/qualitygate/semantic/client.gointernal/qualitygate/semantic/gatekeeper.gointernal/qualitygate/semantic/gatekeeper_test.gointernal/qualitygate/semantic/prompt.gointernal/qualitygate/semantic/prompt_contract_test.gointernal/qualitygate/semantic/schema.gointernal/qualitygate/semantic/scope.gointernal/qualitygate/semantic/scope_test.gointernal/qualitygate/semantic/view.gointernal/qualitygate/semantic/view_test.gointernal/qualitygate/semantic/waiver.gointernal/qualitygate/semantic/waiver_test.goscripts/ci-quality-summary-publish.jsscripts/ci-quality-summary-publish.test.jsscripts/ci-workflow.test.shscripts/semantic-review-publish.jsscripts/semantic-review-publish.test.jsscripts/semantic-review-verify-artifact.jsscripts/semantic-review-verify-artifact.test.jsscripts/semantic-review-workflow.test.sh
✅ Files skipped from review due to trivial changes (2)
- internal/qualitygate/semantic/scope_test.go
- internal/qualitygate/semantic/prompt_contract_test.go
🚧 Files skipped from review as they are similar to previous changes (40)
- internal/qualitygate/publiccontent/comment_audit_test.go
- internal/qualitygate/publiccontent/comment_audit.go
- scripts/ci-quality-summary-publish.js
- internal/qualitygate/semantic/waiver.go
- internal/qualitygate/publiccontent/metadata_test.go
- internal/qualitygate/semantic/schema.go
- internal/qualitygate/semantic/client.go
- internal/qualitygate/publiccontent/metadata.go
- internal/qualitygate/facts/schema.go
- internal/qualitygate/cmd/quality-gate/main.go
- internal/qualitygate/semantic/scope.go
- internal/qualitygate/semantic/prompt.go
- internal/qualitygate/config/semantic/policy.json
- internal/qualitygate/cmd/comment-audit/main_test.go
- .github/workflows/comment-audit.yml
- Makefile
- internal/qualitygate/cmd/quality-gate/main_test.go
- scripts/semantic-review-verify-artifact.test.js
- internal/qualitygate/semantic/view_test.go
- internal/qualitygate/semantic/gatekeeper.go
- internal/qualitygate/facts/schema_test.go
- internal/qualitygate/semantic/gatekeeper_test.go
- scripts/ci-quality-summary-publish.test.js
- internal/qualitygate/rules/run_test.go
- internal/qualitygate/cmd/semantic-review/main_test.go
- .github/workflows/semantic-review.yml
- scripts/semantic-review-verify-artifact.js
- internal/qualitygate/rules/run.go
- scripts/semantic-review-publish.test.js
- internal/qualitygate/cmd/comment-audit/main.go
- scripts/semantic-review-workflow.test.sh
- internal/qualitygate/publiccontent/rules.go
- internal/qualitygate/rules/dryrun.go
- internal/qualitygate/rules/dryrun_test.go
- internal/qualitygate/cmd/semantic-review/main.go
- internal/qualitygate/semantic/view.go
- internal/qualitygate/publiccontent/collect.go
- internal/qualitygate/publiccontent/scan.go
- internal/qualitygate/publiccontent/collect_test.go
- internal/qualitygate/publiccontent/scan_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1577 +/- ##
==========================================
+ Coverage 74.59% 74.72% +0.12%
==========================================
Files 793 799 +6
Lines 79085 80274 +1189
==========================================
+ Hits 58997 59983 +986
- Misses 15702 15846 +144
- Partials 4386 4445 +59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b5b71d9 to
50db467
Compare
Summary
Add public-content safeguards to the CLI quality gate and CI workflow.
This change introduces deterministic scanning for public-content leakage across PR metadata, changed files, branch metadata, and published comments. Findings are wired into CI results, facts artifacts, PR summaries, and semantic review inputs with redaction applied before evidence is persisted or surfaced.
Changes
Validation
make quality-gatemake script-testgo test ./internal/qualitygate/...Private sandbox E2E validation was also run for the new public-content boundaries, including credential variant rejection, benign token-field pass cases, and placeholder pass cases.
Summary by CodeRabbit