feat: add CI quality gates for CLI contracts#1488
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis change adds deterministic quality-gate fact generation, new manifest and facts contracts, repository rule execution, semantic review policy and publishing logic, incremental lint enforcement, and CI workflow wiring that verifies artifacts and publishes PR review summaries and checks. ChangesQuality gate and semantic review rollout
Sequence Diagram(s)sequenceDiagram
participant CI
participant Artifact
participant SemanticWorkflow
participant GitHub
CI->>Artifact: Upload quality-gate facts
SemanticWorkflow->>Artifact: Download and verify facts zip
SemanticWorkflow->>SemanticWorkflow: Run semantic-review CLI
SemanticWorkflow->>GitHub: Publish check and PR summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1488 +/- ##
==========================================
+ Coverage 73.42% 73.46% +0.04%
==========================================
Files 753 782 +29
Lines 70111 74484 +4373
==========================================
+ Hits 51476 54720 +3244
- Misses 14826 15671 +845
- Partials 3809 4093 +284 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a385d9c53cfaefca86fb8cedafe18e491eadd25f🧩 Skill updatenpx skills add larksuite/cli#feat/quality-gate-ci -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
.github/workflows/ci.yml-97-99 (1)
97-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
persist-credentials: falseto prevent credential leakage.The checkout step in
deterministic-gateshould disable credential persistence to prevent Git credentials from being included in uploaded artifacts.Proposed fix
- 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 97 - 99, The checkout action in the deterministic-gate job is missing the persist-credentials security configuration. Add persist-credentials: false to the with block of the actions/checkout step to prevent Git credentials from being persisted and potentially leaking through uploaded artifacts.Source: Linters/SAST tools
internal/qualitygate/semantic/io_test.go-24-35 (1)
24-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a regression test for
InfrastructureFailureDecision.
io.goadds distinct behavior for infrastructure failures (critical severity + flags), but this file doesn’t assert that contract yet. A focused test would prevent silent drift.As per coding guidelines, every behavior change should be covered by tests in the same change.
🤖 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/semantic/io_test.go` around lines 24 - 35, Add a new regression test function called TestInfrastructureFailureDecision that validates the contract for the InfrastructureFailureDecision function. The test should call InfrastructureFailureDecision with an appropriate error and assert that the returned decision has InfrastructureFailure set to true, Degraded set to true, and includes critical severity handling or system warnings as appropriate. Also verify that the decision does not include fake evidence by asserting that Warnings and Blockers are empty, following the same pattern as the existing TestDegradedDecisionUsesSystemWarning test.Source: Coding guidelines
scripts/ci-quality-summary-publish.js-173-179 (1)
173-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBind the fetched jobs to the triggering workflow run.
At Line 178, jobs are fetched from
target.runId, but there is no check that this matchescontext.payload.workflow_run.id. If the env value is stale/miswired, the script can publish a summary for the wrong run.Proposed fix
async function publish({ github, context, core }) { const run = context.payload.workflow_run; if (!run || run.event !== "pull_request") { core.notice("PR quality summary skipped: workflow_run is not a pull_request run"); return; } const target = verifiedPublishTarget(); + if (String(run.id) !== String(target.runId)) { + core.notice("PR quality summary skipped: verified run id does not match triggering workflow_run"); + return; + } if (!(await publishTargetStillCurrent(github, context, core, target))) { return; } const jobs = await listWorkflowRunJobs(github, context, target.runId);🤖 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 `@scripts/ci-quality-summary-publish.js` around lines 173 - 179, Add a validation check to ensure the jobs being fetched correspond to the correct workflow run. After the call to listWorkflowRunJobs where target.runId is passed, verify that target.runId matches context.payload.workflow_run.id. If they do not match, return early from the function to prevent publishing a summary for the wrong workflow run, preventing the risk of stale or misconfigured environment values from publishing incorrect data.internal/qualitygate/semantic/waiver.go-49-55 (1)
49-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the actual input filename in parse errors for
LoadWaiversFile.
LoadWaiversFilecurrently returns parse errors labeled asqualitygate/semantic/waivers.txt, even when another file is passed in. That makes troubleshooting the explicit-file path flow misleading.Proposed fix
-func LoadWaivers(repo string, now time.Time) (Waivers, []report.Diagnostic, error) { +func LoadWaivers(repo string, now time.Time) (Waivers, []report.Diagnostic, error) { data, err := vfs.ReadFile(filepath.Join(repo, filepath.FromSlash(waiverPath))) if err != nil { if missingFile(err) { return Waivers{}, nil, nil } return Waivers{}, nil, err } - return ParseWaivers(strings.NewReader(string(data)), now) + return ParseWaivers(waiverPath, strings.NewReader(string(data)), now) } func LoadWaiversFile(file string, now time.Time) (Waivers, []report.Diagnostic, error) { data, err := vfs.ReadFile(file) if err != nil { return Waivers{}, nil, err } - return ParseWaivers(strings.NewReader(string(data)), now) + return ParseWaivers(file, strings.NewReader(string(data)), now) } -func ParseWaivers(r *strings.Reader, now time.Time) (Waivers, []report.Diagnostic, error) { +func ParseWaivers(fileLabel string, r *strings.Reader, now time.Time) (Waivers, []report.Diagnostic, error) { scanner := bufio.NewScanner(r) @@ - if len(parts) != 10 { - return Waivers{}, diags, fmt.Errorf("%s:%d: expected 10 TSV columns", waiverPath, lineNo) + if len(parts) != 10 { + return Waivers{}, diags, fmt.Errorf("%s:%d: expected 10 TSV columns", fileLabel, lineNo) } - item, err := parseWaiver(parts, lineNo) + item, err := parseWaiver(fileLabel, parts, lineNo)Also applies to: 57-69, 98-149
🤖 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/semantic/waiver.go` around lines 49 - 55, The LoadWaiversFile function at lines 49-55 calls ParseWaivers with only a reader and timestamp, but ParseWaivers must be defaulting to a hardcoded filename in error messages instead of using the actual file parameter passed to LoadWaiversFile. Modify the function to pass the actual filename (the file parameter) to ParseWaivers so that parse errors reflect the correct file being processed. Apply the same fix to the other affected functions at lines 57-69 and 98-149 to ensure all callers that load waivers from explicit file paths produce error messages with the correct filename context.internal/qualitygate/semantic/scope_test.go-43-44 (1)
43-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid panic-prone indexing in the blocker assertion.
This assertion can panic if one blocker is returned with an empty
RolloutGroups, which hides the real regression signal.Proposed fix
- if len(got.Blockers) != 1 || got.Blockers[0].RolloutGroups[0] != "changed-only" { + if len(got.Blockers) != 1 { + t.Fatalf("expected changed-only blocker, got %#v", got) + } + if len(got.Blockers[0].RolloutGroups) != 1 || got.Blockers[0].RolloutGroups[0] != "changed-only" { t.Fatalf("expected changed-only blocker, got %#v", got) }🤖 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/semantic/scope_test.go` around lines 43 - 44, The assertion in the test is accessing got.Blockers[0].RolloutGroups[0] without verifying that RolloutGroups has any elements, which will cause a panic if a blocker is returned with an empty RolloutGroups slice. Add a bounds check to ensure that got.Blockers[0].RolloutGroups has at least one element before accessing index 0, or restructure the condition to safely verify both that there is one blocker and that the first blocker's RolloutGroups contains "changed-only" without risking a panic on empty slices.
🧹 Nitpick comments (3)
.github/workflows/semantic-review.yml (1)
202-202: ⚖️ Poor tradeoffConsider using environment variables instead of direct template interpolation in shell commands.
While the interpolated values are controlled (workflow-constructed paths and GitHub API responses), passing them through environment variables is a safer pattern that avoids potential shell metacharacter issues.
Example refactor
- name: Verify and extract summary facts artifact if: ${{ steps.pr.outputs.stale != 'true' && steps.download.outputs.zip_path != '' }} env: SEMANTIC_REVIEW_BLOCK: ${{ vars.SEMANTIC_REVIEW_BLOCK }} SEMANTIC_REVIEW_DECISION_OUT: decision.json SEMANTIC_REVIEW_MARKDOWN_OUT: semantic-review.md + ARTIFACT_ZIP_PATH: ${{ steps.download.outputs.zip_path }} + ARTIFACT_DIGEST: ${{ steps.artifact.outputs.artifact_digest }} - run: node scripts/semantic-review-verify-artifact.js '${{ steps.download.outputs.zip_path }}' facts.json '${{ steps.artifact.outputs.artifact_digest }}' + run: node scripts/semantic-review-verify-artifact.js "$ARTIFACT_ZIP_PATH" facts.json "$ARTIFACT_DIGEST"🤖 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/semantic-review.yml at line 202, The shell command in the run step is directly interpolating GitHub Actions template expressions instead of using environment variables, which is less safe. Convert the run step to first set environment variables from the step outputs (steps.download.outputs.zip_path and steps.artifact.outputs.artifact_digest), then reference those environment variables in the node command invocation using the $VARIABLE_NAME syntax rather than direct template interpolation.internal/qualitygate/rules/run_test.go (1)
16-17: ⚡ Quick winPrefer stdlib filesystem APIs for test fixture setup (not
internal/vfs).These fixture writes are in
_test.goundert.TempDir(), so useos.MkdirAll/os.WriteFileto keep tests decoupled from production VFS behavior.Proposed refactor
import ( "context" "encoding/json" + "os" "os/exec" "path/filepath" "testing" @@ - "github.com/larksuite/cli/internal/vfs" ) @@ - if err := vfs.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir golden dir: %v", err) } - if err := vfs.WriteFile(path, append(data, '\n'), 0o644); err != nil { + if err := os.WriteFile(path, append(data, '\n'), 0o644); err != nil { t.Fatalf("write golden: %v", err) } @@ - if err := vfs.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir golden dir: %v", err) } - if err := vfs.WriteFile(path, append(data, '\n'), 0o644); err != nil { + if err := os.WriteFile(path, append(data, '\n'), 0o644); err != nil { t.Fatalf("write golden: %v", err) } @@ - if err := vfs.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir golden dir: %v", err) } - if err := vfs.WriteFile(path, nil, 0o644); err != nil { + if err := os.WriteFile(path, nil, 0o644); err != nil { t.Fatalf("write empty golden: %v", err) }Based on learnings: in this repo’s
_test.gofiles, fixture setup undert.TempDir()should use stdlibos.*APIs rather thanvfs.*.Also applies to: 85-89, 126-130, 156-160
🤖 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/rules/run_test.go` around lines 16 - 17, Remove the import of internal/vfs from this test file and replace all fixture setup calls that use vfs methods with their stdlib equivalents. Specifically, replace vfs directory creation calls with os.MkdirAll and vfs file writing calls with os.WriteFile. This change should be applied throughout the test file where fixture setup occurs: at the import section (lines 16-17 anchor), and at all fixture setup locations around lines 85-89, 126-130, and 156-160 where vfs methods are currently being used to create test fixtures under t.TempDir().Source: Learnings
internal/qualitygate/examples/from_manifest_test.go (1)
18-23: ⚡ Quick winAssert the full
skillscan.Examplecontract in this test.The test currently allows regressions in
Raw,Line, andHasPlaceholdermapping to slip through. Adding explicit field assertions for both examples will lock the behavior more reliably.Suggested test expansion
func TestHarvestManifestExamples(t *testing.T) { m := manifest.Manifest{Commands: []manifest.Command{{ Path: "root", Example: "lark-cli calendar +agenda\nlark-cli api GET /open-apis/test", }}} got := FromManifest(m) if len(got) != 2 { t.Fatalf("got %d examples, want 2", len(got)) } if got[0].SourceFile != "command-manifest" { t.Fatalf("source = %q", got[0].SourceFile) } + if got[0].Raw != "lark-cli calendar +agenda" || got[0].Line != 1 || got[0].HasPlaceholder { + t.Fatalf("example[0] = %#v", got[0]) + } + if got[1].Raw != "lark-cli api GET /open-apis/test" || got[1].Line != 2 || got[1].HasPlaceholder { + t.Fatalf("example[1] = %#v", got[1]) + } }🤖 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/examples/from_manifest_test.go` around lines 18 - 23, The test currently only validates the count of examples and the SourceFile field of the first example, leaving the Raw, Line, and HasPlaceholder fields unchecked for both examples. Add explicit assertions in this test to verify that the Raw, Line, and HasPlaceholder fields are correctly populated for both got[0] and got[1] examples to prevent regressions in the manifest parsing behavior and ensure the full skillscan.Example contract is being tested.
🤖 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 `@cmd/build_test.go`:
- Around line 15-27: The test TestBuildWithoutPluginsStillBuildsBuiltinCommands
currently only covers the WithoutPlugins() build option, but the Build function
in cmd/build.go introduced additional behavior-changing options including
WithoutStrictMode(), WithoutServiceCommands(), and WithServiceCatalog(...) that
lack direct test coverage. Add separate test functions for each of these build
options (following the same pattern as the existing test) to verify that each
option produces the expected behavior and commands, ensuring every
behavior-changing path has corresponding test coverage.
In `@cmd/build.go`:
- Around line 112-115: The WithServiceCatalog function stores a pointer to the
input catalog without checking if it is nil, which creates a non-nil pointer
pointing to nil. This causes downstream code to incorrectly treat it as a valid
catalog and pass the nil value to RegisterServiceCommandsFromCatalog, triggering
a panic. Add a guard check at the beginning of the WithServiceCatalog function
to verify the catalog parameter is not nil before assigning it to
c.serviceCatalog, otherwise leave c.serviceCatalog as nil to prevent the panic.
In `@internal/qualitygate/allowlist/legacy.go`:
- Around line 39-55: Trim whitespace from all parsed columns before validation
and struct assignment in the legacy command parser. After splitting the text by
tab delimiter, apply strings.TrimSpace to parts[0], parts[1], parts[2], and
parts[3] before performing the blank check, date parsing with time.Parse, and
creating the LegacyCommand struct. This ensures that leading/trailing whitespace
does not cause silent mismatches in downstream exact key lookups. Apply the same
trimming logic to the other parser mentioned in the comment (the one around
lines 77-94 that handles a similar parsing pattern).
In `@internal/qualitygate/diff/diff.go`:
- Line 27: The current implementation uses strings.Fields to parse git output,
which breaks for filenames containing whitespace and can misclassify changed
scope. Add the -z flag to the git diff command in the gitChangedFiles call at
line 27 to request NUL-delimited output, then update all output parsing logic at
lines 40-43 and 60-62 to handle NUL-delimited strings instead of using
strings.Fields, ensuring file paths with spaces are correctly preserved and
processed.
In `@internal/qualitygate/facts/schema.go`:
- Around line 385-399: The domainFromSkillPath function extracts a domain from
the skill path but does not normalize it before checking against knownDomains,
causing aliases like "doc" to fail lookup instead of mapping to "docs". Apply
the normalizeCommandDomain function to the extracted domain variable before
performing the knownDomains lookup to ensure domain aliases are properly
resolved and domain enrichment is preserved for all skill paths.
In `@internal/qualitygate/facts/write.go`:
- Around line 13-35: The WriteFile and ReadFile functions in the facts
persistence code lack both path validation and typed error handling. In
WriteFile, add validate.SafeOutputPath to validate the path parameter before any
filesystem operations, and wrap all error returns (from json.MarshalIndent,
vfs.MkdirAll, and vfs.WriteFile) with errs.NewInternalError(errs.SubtypeFileIO,
...).WithCause(err). Similarly, in ReadFile, add validate.SafeInputPath to
validate the path parameter at the start, and wrap all error returns (from
vfs.ReadFile and json.Unmarshal) with errs.NewInternalError(errs.SubtypeFileIO,
...).WithCause(err) to preserve the underlying error details while providing
typed error handling.
In `@internal/qualitygate/report/report_test.go`:
- Around line 8-21: The test file currently only validates the ExitCode function
but lacks test coverage for the newly introduced Print function. Add a new test
function that exercises the Print function's behavior and includes assertions to
verify two key aspects: first, that the output is deterministically sorted (not
in arbitrary order), and second, that hint lines are conditionally formatted and
included based on the diagnostic data. This test should complement the existing
TestExitCodeRejectOnly test and ensure the Print function's contract is properly
validated.
In `@internal/qualitygate/rules/skillquality_test.go`:
- Around line 8-21: The test file currently only includes
TestSkillQualityWarnsExcessiveCritical which tests the skill_critical_noise
rule, but the change also introduces skill_description_route_quality,
skill_size_budget rules and LoadSkillDocs behavior that lack test coverage. Add
focused test functions for each of these new behaviors in the same file,
following the same pattern as TestSkillQualityWarnsExcessiveCritical: create
SkillDoc instances that trigger each rule, call CheckSkillQuality, and assert
that the expected diagnostics and facts are returned. Ensure the tests validate
the specific rule names and fact conditions for each behavior.
In `@internal/qualitygate/semantic/client_test.go`:
- Around line 195-212: In the error validation test for the Review() method,
keep the existing substring checks for redaction and context verification, but
add typed error contract assertions. After the substring validation loop, use
errs.ProblemOf to assert problem metadata fields (category and subtype), use
errors.Is and errors.As to verify cause preservation in the error chain, and
specifically use errors.As with *errs.ValidationError to assert the Param field
rather than relying on errs.ProblemOf for parameter validation. Apply these same
typed assertion patterns to all failure-path tests that validate error behavior.
In `@internal/qualitygate/semantic/config.go`:
- Around line 38-41: The `LoadPolicy` function (and similarly `LoadModelConfig`)
reads from file paths derived from the untrusted `repo` parameter without path
validation and without wrapping errors in typed internal error types. Before
calling `vfs.ReadFile`, validate the resolved file path using
`validate.SafeInputPath` to ensure path safety. Additionally, replace the bare
error return with a typed internal error (using the appropriate `errs.*`
function) that wraps the file-read error by calling `.WithCause(err)` to
preserve the original error cause for debugging. Apply this same pattern to both
the `LoadPolicy` function at lines 38-41 and the `LoadModelConfig` function at
lines 116-120.
In `@internal/qualitygate/semantic/io.go`:
- Around line 35-38: The code is performing file read and write operations on
user-provided paths without validation, creating a path traversal vulnerability.
Before the vfs.ReadFile call using reviewPath, add validation using
validate.SafeInputPath to ensure the path is safe. Similarly, before all
vfs.WriteFile operations on the path variable (at the locations mentioned as
also affected), add validation using validate.SafeOutputPath to ensure the
output paths are safe. This should be done for all three affected locations in
io.go to comply with the coding guidelines for user-facing file operations.
In `@internal/qualitygate/semantic/schema.go`:
- Around line 99-103: The decoder is accepting inputs with trailing JSON tokens,
which violates the single-document contract. After calling dec.Decode(&raw) at
lines 99-103 and at the second decode location around lines 132-134, add a check
using the decoder's More() method to verify that no additional JSON tokens
remain in the input stream. If More() returns true, return an error indicating
unexpected trailing content. Apply this validation check at both decode
locations to ensure malformed model output with extra tokens is properly
rejected.
In `@internal/qualitygate/semantic/view.go`:
- Around line 104-108: The InputView struct is computing RuleSummary using the
unfiltered f.Diagnostics instead of the filtered viewDiagnostics variable that
was intentionally created in lines 92-102. Change the ruleSummary(f.Diagnostics)
call to ruleSummary(viewDiagnostics) to ensure the rule summary uses the same
filtered diagnostics as the rest of the view, preventing non-semantic
diagnostics from biasing the result.
In `@internal/qualitygate/semantic/waiver_test.go`:
- Around line 11-73: The PR introduces the LoadWaiversFile function but the test
file only contains tests for LoadWaivers and TestLoadWaiversExpiresRows. Add a
new test function (such as TestLoadWaiversFile) that directly tests the
LoadWaiversFile function behavior, including cases for successful file parsing
and error conditions with proper error path attribution. Follow the same
patterns used in the existing TestLoadWaivers function by testing both valid and
invalid input scenarios to ensure the new LoadWaiversFile behavior contract is
properly covered.
In `@lint/errscontract/scan.go`:
- Around line 193-196: Replace the direct `os.ReadFile` calls with the
filesystem abstraction from `internal/vfs` package in both locations. In the
`LegacyCommandErrorCandidatesForRepo` function (lines 193-196) and in the
`LoadLegacyCommandErrorAllowlistWithDiagnostics` function (lines 220-226),
import the `internal/vfs` package and replace each `os.ReadFile(path)` call with
the equivalent method from the vfs abstraction to comply with the repo's
filesystem access contract and enable proper test mocking support.
- Around line 235-249: The changedFilesFrom function currently uses
strings.Fields to tokenize the git diff output, which splits on any whitespace
including spaces and tabs within filenames. This breaks filenames containing
spaces by treating them as multiple separate files. Replace the whitespace-based
tokenization with line-by-line parsing by splitting on newlines instead, since
git diff --name-only outputs one filename per line. Update the loop that
iterates over the parsed output to work with the line-based split result.
In `@scripts/semantic-review-verify-artifact.js`:
- Around line 448-451: The `writeVerifiedFacts` function unconditionally calls
`verifyArtifactDigest(buf, expectedDigest)` even though the expectedDigest
parameter is optional with a default empty string value, and
verifyArtifactDigest throws on empty digest. This breaks the optional contract
and causes invocations without a digest argument to fail unexpectedly. Fix this
by wrapping the `verifyArtifactDigest` call in a conditional check that only
executes when expectedDigest is a non-empty string.
---
Minor comments:
In @.github/workflows/ci.yml:
- Around line 97-99: The checkout action in the deterministic-gate job is
missing the persist-credentials security configuration. Add persist-credentials:
false to the with block of the actions/checkout step to prevent Git credentials
from being persisted and potentially leaking through uploaded artifacts.
In `@internal/qualitygate/semantic/io_test.go`:
- Around line 24-35: Add a new regression test function called
TestInfrastructureFailureDecision that validates the contract for the
InfrastructureFailureDecision function. The test should call
InfrastructureFailureDecision with an appropriate error and assert that the
returned decision has InfrastructureFailure set to true, Degraded set to true,
and includes critical severity handling or system warnings as appropriate. Also
verify that the decision does not include fake evidence by asserting that
Warnings and Blockers are empty, following the same pattern as the existing
TestDegradedDecisionUsesSystemWarning test.
In `@internal/qualitygate/semantic/scope_test.go`:
- Around line 43-44: The assertion in the test is accessing
got.Blockers[0].RolloutGroups[0] without verifying that RolloutGroups has any
elements, which will cause a panic if a blocker is returned with an empty
RolloutGroups slice. Add a bounds check to ensure that
got.Blockers[0].RolloutGroups has at least one element before accessing index 0,
or restructure the condition to safely verify both that there is one blocker and
that the first blocker's RolloutGroups contains "changed-only" without risking a
panic on empty slices.
In `@internal/qualitygate/semantic/waiver.go`:
- Around line 49-55: The LoadWaiversFile function at lines 49-55 calls
ParseWaivers with only a reader and timestamp, but ParseWaivers must be
defaulting to a hardcoded filename in error messages instead of using the actual
file parameter passed to LoadWaiversFile. Modify the function to pass the actual
filename (the file parameter) to ParseWaivers so that parse errors reflect the
correct file being processed. Apply the same fix to the other affected functions
at lines 57-69 and 98-149 to ensure all callers that load waivers from explicit
file paths produce error messages with the correct filename context.
In `@scripts/ci-quality-summary-publish.js`:
- Around line 173-179: Add a validation check to ensure the jobs being fetched
correspond to the correct workflow run. After the call to listWorkflowRunJobs
where target.runId is passed, verify that target.runId matches
context.payload.workflow_run.id. If they do not match, return early from the
function to prevent publishing a summary for the wrong workflow run, preventing
the risk of stale or misconfigured environment values from publishing incorrect
data.
---
Nitpick comments:
In @.github/workflows/semantic-review.yml:
- Line 202: The shell command in the run step is directly interpolating GitHub
Actions template expressions instead of using environment variables, which is
less safe. Convert the run step to first set environment variables from the step
outputs (steps.download.outputs.zip_path and
steps.artifact.outputs.artifact_digest), then reference those environment
variables in the node command invocation using the $VARIABLE_NAME syntax rather
than direct template interpolation.
In `@internal/qualitygate/examples/from_manifest_test.go`:
- Around line 18-23: The test currently only validates the count of examples and
the SourceFile field of the first example, leaving the Raw, Line, and
HasPlaceholder fields unchecked for both examples. Add explicit assertions in
this test to verify that the Raw, Line, and HasPlaceholder fields are correctly
populated for both got[0] and got[1] examples to prevent regressions in the
manifest parsing behavior and ensure the full skillscan.Example contract is
being tested.
In `@internal/qualitygate/rules/run_test.go`:
- Around line 16-17: Remove the import of internal/vfs from this test file and
replace all fixture setup calls that use vfs methods with their stdlib
equivalents. Specifically, replace vfs directory creation calls with os.MkdirAll
and vfs file writing calls with os.WriteFile. This change should be applied
throughout the test file where fixture setup occurs: at the import section
(lines 16-17 anchor), and at all fixture setup locations around lines 85-89,
126-130, and 156-160 where vfs methods are currently being used to create test
fixtures under t.TempDir().
🪄 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: 5e56117a-de98-4417-beee-7289ffc5b733
📒 Files selected for processing (89)
.github/workflows/ci.yml.github/workflows/semantic-review.yml.gitignoreMakefilecmd/build.gocmd/build_test.gocmd/service/service.gointernal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/qualitygate/allowlist/legacy.gointernal/qualitygate/allowlist/legacy_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/examples/from_manifest.gointernal/qualitygate/examples/from_manifest_test.gointernal/qualitygate/facts/schema.gointernal/qualitygate/facts/schema_test.gointernal/qualitygate/facts/write.gointernal/qualitygate/manifest/collect.gointernal/qualitygate/manifest/collect_test.gointernal/qualitygate/manifest/schema.gointernal/qualitygate/report/report.gointernal/qualitygate/report/report_test.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/errorfacts.gointernal/qualitygate/rules/errorfacts_test.gointernal/qualitygate/rules/naming.gointernal/qualitygate/rules/naming_test.gointernal/qualitygate/rules/output.gointernal/qualitygate/rules/output_test.gointernal/qualitygate/rules/refs.gointernal/qualitygate/rules/refs_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/qualitygate/rules/skillquality.gointernal/qualitygate/rules/skillquality_test.gointernal/qualitygate/semantic/ark_live_test.gointernal/qualitygate/semantic/client.gointernal/qualitygate/semantic/client_test.gointernal/qualitygate/semantic/config.gointernal/qualitygate/semantic/config_test.gointernal/qualitygate/semantic/gatekeeper.gointernal/qualitygate/semantic/gatekeeper_test.gointernal/qualitygate/semantic/io.gointernal/qualitygate/semantic/io_test.gointernal/qualitygate/semantic/prompt.gointernal/qualitygate/semantic/prompt_contract_test.gointernal/qualitygate/semantic/schema.gointernal/qualitygate/semantic/schema_test.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.gointernal/qualitygate/skillscan/harvest.gointernal/qualitygate/skillscan/harvest_test.gointernal/qualitygate/skillscan/testdata/skills/lark-demo/SKILL.mdlint/errscontract/rule_no_bare_command_error.golint/errscontract/rule_no_bare_command_error_test.golint/errscontract/runner.golint/errscontract/scan.golint/errscontract/scan_test.golint/main.goqualitygate/README.mdqualitygate/allowlists/legacy-command-errors.txtqualitygate/allowlists/legacy-commands.txtqualitygate/allowlists/legacy-flags.txtqualitygate/semantic/models.jsonqualitygate/semantic/policy.jsonqualitygate/semantic/waivers.txtscripts/ci-quality-summary-publish.jsscripts/ci-quality-summary-publish.test.jsscripts/ci-workflow.test.shscripts/pr-quality-summary.jsscripts/pr-quality-summary.test.jsscripts/resolve-changed-from.shscripts/resolve-changed-from.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.shshortcuts/common/runner.gotools/quality-gate/main.gotools/quality-gate/main_test.gotools/semantic-review/main.gotools/semantic-review/main_test.go
HanShaoshuai-k
force-pushed
the
feat/quality-gate-ci
branch
from
e9b5ca4 to
ce80b29
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 97-99: Add `persist-credentials: false` to the `with:` section of
the `actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5` action in
the deterministic-gate job. This option should be added alongside the existing
`fetch-depth: 0` parameter to prevent Git credentials from being persisted in
uploaded artifacts, matching the security pattern already applied in the
semantic-review.yml workflow.
🪄 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: 15f0200f-3cf5-4d60-b5c5-bcaaf426f2ad
📒 Files selected for processing (93)
.github/workflows/ci.yml.github/workflows/semantic-review.yml.gitignoreMakefilecmd/build.gocmd/build_test.gocmd/service/service.gointernal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/qualitygate/allowlist/legacy.gointernal/qualitygate/allowlist/legacy_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/examples/from_manifest.gointernal/qualitygate/examples/from_manifest_test.gointernal/qualitygate/facts/schema.gointernal/qualitygate/facts/schema_test.gointernal/qualitygate/facts/write.gointernal/qualitygate/manifest/collect.gointernal/qualitygate/manifest/collect_test.gointernal/qualitygate/manifest/schema.gointernal/qualitygate/report/report.gointernal/qualitygate/report/report_test.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/errorfacts.gointernal/qualitygate/rules/errorfacts_test.gointernal/qualitygate/rules/naming.gointernal/qualitygate/rules/naming_test.gointernal/qualitygate/rules/output.gointernal/qualitygate/rules/output_test.gointernal/qualitygate/rules/refs.gointernal/qualitygate/rules/refs_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/qualitygate/rules/skillquality.gointernal/qualitygate/rules/skillquality_test.gointernal/qualitygate/semantic/ark_live_test.gointernal/qualitygate/semantic/client.gointernal/qualitygate/semantic/client_test.gointernal/qualitygate/semantic/config.gointernal/qualitygate/semantic/config_test.gointernal/qualitygate/semantic/gatekeeper.gointernal/qualitygate/semantic/gatekeeper_test.gointernal/qualitygate/semantic/io.gointernal/qualitygate/semantic/io_test.gointernal/qualitygate/semantic/prompt.gointernal/qualitygate/semantic/prompt_contract_test.gointernal/qualitygate/semantic/schema.gointernal/qualitygate/semantic/schema_test.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.gointernal/qualitygate/skillscan/harvest.gointernal/qualitygate/skillscan/harvest_test.gointernal/qualitygate/skillscan/testdata/skills/lark-demo/SKILL.mdinternal/vfs/default.gointernal/vfs/fs.gointernal/vfs/osfs.gointernal/vfs/osfs_test.golint/errscontract/rule_no_bare_command_error.golint/errscontract/rule_no_bare_command_error_test.golint/errscontract/runner.golint/errscontract/scan.golint/errscontract/scan_test.golint/main.goqualitygate/README.mdqualitygate/allowlists/legacy-command-errors.txtqualitygate/allowlists/legacy-commands.txtqualitygate/allowlists/legacy-flags.txtqualitygate/semantic/models.jsonqualitygate/semantic/policy.jsonqualitygate/semantic/waivers.txtscripts/ci-quality-summary-publish.jsscripts/ci-quality-summary-publish.test.jsscripts/ci-workflow.test.shscripts/pr-quality-summary.jsscripts/pr-quality-summary.test.jsscripts/resolve-changed-from.shscripts/resolve-changed-from.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.shshortcuts/common/runner.gotools/quality-gate/main.gotools/quality-gate/main_test.gotools/semantic-review/main.gotools/semantic-review/main_test.go
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- internal/qualitygate/skillscan/testdata/skills/lark-demo/SKILL.md
- qualitygate/semantic/waivers.txt
- internal/qualitygate/semantic/config_test.go
🚧 Files skipped from review as they are similar to previous changes (66)
- internal/qualitygate/examples/from_manifest_test.go
- internal/qualitygate/facts/write.go
- lint/errscontract/runner.go
- qualitygate/allowlists/legacy-command-errors.txt
- qualitygate/semantic/models.json
- internal/qualitygate/report/report_test.go
- qualitygate/allowlists/legacy-commands.txt
- internal/qualitygate/manifest/schema.go
- internal/qualitygate/diff/diff_test.go
- qualitygate/semantic/policy.json
- internal/qualitygate/semantic/prompt_contract_test.go
- internal/qualitygate/rules/skillquality_test.go
- cmd/build_test.go
- internal/qualitygate/semantic/schema_test.go
- internal/qualitygate/allowlist/legacy_test.go
- internal/qualitygate/report/report.go
- internal/qualitygate/semantic/prompt.go
- internal/cmdmeta/meta_test.go
- internal/cmdmeta/meta.go
- internal/qualitygate/semantic/waiver_test.go
- internal/qualitygate/rules/output_test.go
- internal/qualitygate/allowlist/legacy.go
- scripts/resolve-changed-from.test.sh
- lint/main.go
- internal/qualitygate/manifest/collect_test.go
- internal/qualitygate/facts/schema_test.go
- internal/qualitygate/rules/output.go
- lint/errscontract/scan_test.go
- internal/qualitygate/examples/from_manifest.go
- scripts/resolve-changed-from.sh
- internal/qualitygate/semantic/io.go
- internal/qualitygate/rules/skillquality.go
- scripts/ci-quality-summary-publish.js
- internal/qualitygate/diff/diff.go
- internal/qualitygate/semantic/scope_test.go
- lint/errscontract/scan.go
- scripts/pr-quality-summary.test.js
- internal/qualitygate/semantic/client.go
- internal/qualitygate/semantic/io_test.go
- lint/errscontract/rule_no_bare_command_error_test.go
- cmd/build.go
- internal/qualitygate/skillscan/harvest_test.go
- internal/qualitygate/semantic/scope.go
- lint/errscontract/rule_no_bare_command_error.go
- internal/qualitygate/semantic/waiver.go
- internal/qualitygate/rules/naming.go
- internal/qualitygate/rules/naming_test.go
- scripts/ci-quality-summary-publish.test.js
- internal/qualitygate/semantic/view.go
- internal/qualitygate/semantic/schema.go
- internal/qualitygate/semantic/config.go
- internal/qualitygate/rules/errorfacts.go
- internal/qualitygate/facts/schema.go
- internal/qualitygate/skillscan/harvest.go
- internal/qualitygate/semantic/gatekeeper_test.go
- internal/qualitygate/semantic/client_test.go
- internal/qualitygate/rules/dryrun.go
- internal/qualitygate/rules/refs.go
- scripts/semantic-review-publish.test.js
- internal/qualitygate/semantic/ark_live_test.go
- internal/qualitygate/rules/run_test.go
- internal/qualitygate/semantic/gatekeeper.go
- internal/qualitygate/rules/errorfacts_test.go
- internal/qualitygate/rules/run.go
- internal/qualitygate/rules/refs_test.go
- internal/qualitygate/rules/dryrun_test.go
HanShaoshuai-k
force-pushed
the
feat/quality-gate-ci
branch
from
ce80b29 to
a385d9c
Compare
tuxedomm
approved these changes
Jun 17, 2026
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/semantic-review.yml:
- Around line 463-469: The semantic review verification in block mode must
validate facts and waivers using only the trusted base implementation, not
potentially modified PR-supplied versions. Modify the workflow logic so that
when SEMANTIC_REVIEW_BLOCK is true, the script regenerates and verifies facts
using the checked-out base implementation instead of relying on the artifacts
from the PR, and loads waivers exclusively from the verified base checkout. If
the trusted path cannot be established or facts cannot be verified against the
base implementation, the check should fail closed rather than proceed with
potentially compromised inputs.
In `@internal/qualitygate/cmd/manifest-export/main.go`:
- Around line 51-58: The user-supplied CLI flag values manifestOut and
commandIndexOut are passed to ensureParentDir() without validation before
filesystem access. Validate both manifestOut and commandIndexOut using
validate.SafeOutputPath before calling ensureParentDir() on each of them. If
validation fails, log an appropriate error and return a non-zero exit code. This
ensures untrusted CLI arguments are validated against security guidelines before
any file write operations occur.
In `@internal/qualitygate/cmd/quality-gate/main.go`:
- Around line 31-88: The runCheck function accepts user-supplied file paths
through CLI flags (--manifest, --command-index, --facts-out) and passes them
directly to file operations without validation, treating them as untrusted
input. Add validate.SafeInputPath calls before passing opts.ManifestPath and
opts.CommandIndexPath to manifest.ReadFile, and add validate.SafeOutputPath
before passing opts.FactsOut to facts.WriteFile. Additionally, replace all bare
fmt.Fprintf error logging calls throughout runCheck with typed
errs.NewValidationError or appropriate error types from the errs package to
provide consistent error handling that aligns with internal CLI patterns.
In `@internal/qualitygate/manifest/io.go`:
- Around line 41-51: The WriteFile function accepts a user-supplied path
parameter that requires validation before being used, and errors from
localfileio.AtomicWrite need to be wrapped with typed errors for consistency.
Add a call to validate.SafeOutputPath to validate the path parameter at the
beginning of the WriteFile function, then wrap the error returned by
localfileio.AtomicWrite with errs.NewInternalError(errs.SubtypeFileIO, ...) to
provide a properly typed error instead of returning the raw error directly.
- Around line 27-39: In the readBytes function, replace the bare fmt.Errorf
calls with typed errors from the errs package. The error returned on line 29
when the manifest exceeds MaxManifestBytes should use an errs typed error
constructor instead of fmt.Errorf, and the same applies to the error returned on
line 32 when JSON unmarshaling fails. Ensure the error messages and context
(including the label and error details) are preserved when converting to typed
errors by using the appropriate errs package constructors that accept formatted
messages and wrapped errors.
- Around line 15-21: The ReadFile function accepts a path parameter from
untrusted CLI sources without validating it for safety. Add input validation at
the beginning of the ReadFile function by calling validate.SafeInputPath on the
path parameter before performing the vfs.ReadFile operation. This will prevent
path traversal and other potential security issues as required by the coding
guidelines.
- Around line 16-18: The error handling in the vfs.ReadFile(path) call needs to
be updated to follow coding guidelines. First, validate the path parameter using
validate.SafeInputPath(path) before attempting to read the file, since the path
comes from user input. Then, wrap the error returned by vfs.ReadFile with
errs.NewInternalError(errs.SubtypeFileIO, ...) and chain it using the
.WithCause() method to attach the original error (note that .WithParam() does
not exist on InternalError, only .WithCause() should be used).
In `@internal/qualitygate/rules/run_test.go`:
- Line 111: Replace all instances of vfs.WriteFile with os.WriteFile and
vfs.MkdirAll with os.MkdirAll in test fixture setup across all test functions in
this file. Specifically, update the fixture setup code in
TestLoadBaseReferenceManifestReadsCommandGolden,
TestLoadBaseReferenceManifestReadsCommandIndexGolden,
TestLoadBaseReferenceManifestRejectsEmptyGolden, and
TestLoadBaseReferenceManifestRejectsInvalidGoldenKind functions. This change
ensures that test fixtures created under t.TempDir() use standard library
filesystem APIs instead of production infrastructure APIs, avoiding circular
test dependencies.
In `@scripts/ci-workflow.test.sh`:
- Around line 23-27: The current test only verifies that deterministic-gate
appears somewhere in the workflow file, but does not independently verify that
both the e2e-live and e2e-staging job definitions include deterministic-gate in
their respective needs lists. To fix this, extract the needs section for
e2e-live separately using the awk pattern (looking for /^ e2e-live:/ and
extracting until the next job section), then verify that deterministic-gate is
present in that extracted needs list. Repeat the same process for e2e-staging
(which appears to be tested around line 106-109). This ensures that each E2E job
independently declares its dependency on deterministic-gate rather than just
checking for the string's presence anywhere in the file.
In `@scripts/pr-quality-summary.js`:
- Around line 9-10: The sanitizeMarkdownBody function uses the `||` operator
which incorrectly treats falsy values like 0 and false as empty, causing loss of
legitimate data in PR summaries. Replace the `||` operator with the nullish
coalescing operator `??` in the sanitizeMarkdownBody function to only fall back
to empty string for actual null or undefined values. Apply this same fix to the
other locations mentioned at lines 34-39, 55-56, and 112-113 where this pattern
is also used.
🪄 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: ef48014f-947f-4f92-a433-5f6b573b76f7
📒 Files selected for processing (97)
.github/workflows/ci.yml.github/workflows/semantic-review.yml.golangci.ymlMakefilecmd/build.gocmd/build_test.gocmd/service/service.gointernal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/qualitygate/allowlist/legacy.gointernal/qualitygate/allowlist/legacy_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main.gointernal/qualitygate/cmd/manifest-export/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/README.mdinternal/qualitygate/config/allowlists/legacy-command-errors.txtinternal/qualitygate/config/allowlists/legacy-commands.txtinternal/qualitygate/config/allowlists/legacy-flags.txtinternal/qualitygate/config/semantic/models.jsoninternal/qualitygate/config/semantic/policy.jsoninternal/qualitygate/config/semantic/waivers.txtinternal/qualitygate/deptest/deptest_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/examples/from_manifest.gointernal/qualitygate/examples/from_manifest_test.gointernal/qualitygate/facts/schema.gointernal/qualitygate/facts/schema_test.gointernal/qualitygate/facts/write.gointernal/qualitygate/manifest/io.gointernal/qualitygate/manifest/io_test.gointernal/qualitygate/manifest/schema.gointernal/qualitygate/report/report.gointernal/qualitygate/report/report_test.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/errorfacts.gointernal/qualitygate/rules/errorfacts_test.gointernal/qualitygate/rules/naming.gointernal/qualitygate/rules/naming_test.gointernal/qualitygate/rules/output.gointernal/qualitygate/rules/output_test.gointernal/qualitygate/rules/refs.gointernal/qualitygate/rules/refs_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/qualitygate/rules/skillquality.gointernal/qualitygate/rules/skillquality_test.gointernal/qualitygate/semantic/ark_live_test.gointernal/qualitygate/semantic/client.gointernal/qualitygate/semantic/client_test.gointernal/qualitygate/semantic/config.gointernal/qualitygate/semantic/config_test.gointernal/qualitygate/semantic/gatekeeper.gointernal/qualitygate/semantic/gatekeeper_test.gointernal/qualitygate/semantic/io.gointernal/qualitygate/semantic/io_test.gointernal/qualitygate/semantic/prompt.gointernal/qualitygate/semantic/prompt_contract_test.gointernal/qualitygate/semantic/schema.gointernal/qualitygate/semantic/schema_test.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.gointernal/qualitygate/skillscan/harvest.gointernal/qualitygate/skillscan/harvest_test.gointernal/qualitygate/skillscan/testdata/skills/lark-demo/SKILL.mdinternal/vfs/default.gointernal/vfs/fs.gointernal/vfs/osfs.gointernal/vfs/osfs_test.golint/errscontract/rule_no_bare_command_error.golint/errscontract/rule_no_bare_command_error_test.golint/errscontract/runner.golint/errscontract/scan.golint/errscontract/scan_test.golint/main.goscripts/ci-quality-summary-publish.jsscripts/ci-quality-summary-publish.test.jsscripts/ci-workflow.test.shscripts/pr-quality-summary.jsscripts/pr-quality-summary.test.jsscripts/resolve-changed-from.shscripts/resolve-changed-from.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.shshortcuts/common/runner.go
💤 Files with no reviewable changes (1)
- scripts/semantic-review-publish.js
✅ Files skipped from review due to trivial changes (8)
- internal/qualitygate/config/semantic/models.json
- internal/qualitygate/config/allowlists/legacy-flags.txt
- internal/qualitygate/skillscan/testdata/skills/lark-demo/SKILL.md
- internal/qualitygate/config/allowlists/legacy-commands.txt
- internal/qualitygate/config/allowlists/legacy-command-errors.txt
- internal/qualitygate/config/semantic/waivers.txt
- .golangci.yml
- internal/qualitygate/cmd/manifest-export/main_test.go
🚧 Files skipped from review as they are similar to previous changes (60)
- internal/qualitygate/facts/write.go
- internal/vfs/default.go
- internal/qualitygate/rules/skillquality_test.go
- internal/qualitygate/semantic/io_test.go
- internal/qualitygate/report/report_test.go
- internal/vfs/osfs.go
- lint/errscontract/runner.go
- internal/cmdmeta/meta_test.go
- internal/qualitygate/examples/from_manifest_test.go
- internal/qualitygate/skillscan/harvest_test.go
- internal/qualitygate/semantic/waiver_test.go
- internal/vfs/fs.go
- internal/vfs/osfs_test.go
- internal/cmdmeta/meta.go
- lint/main.go
- internal/qualitygate/rules/output_test.go
- cmd/build_test.go
- scripts/pr-quality-summary.test.js
- internal/qualitygate/examples/from_manifest.go
- lint/errscontract/scan_test.go
- scripts/resolve-changed-from.test.sh
- scripts/resolve-changed-from.sh
- internal/qualitygate/diff/diff_test.go
- internal/qualitygate/semantic/prompt.go
- internal/qualitygate/report/report.go
- internal/qualitygate/semantic/prompt_contract_test.go
- internal/qualitygate/allowlist/legacy.go
- internal/qualitygate/allowlist/legacy_test.go
- internal/qualitygate/rules/output.go
- internal/qualitygate/rules/skillquality.go
- internal/qualitygate/semantic/scope_test.go
- internal/qualitygate/semantic/schema.go
- cmd/service/service.go
- internal/qualitygate/semantic/waiver.go
- internal/qualitygate/semantic/config.go
- internal/qualitygate/semantic/io.go
- internal/qualitygate/diff/diff.go
- lint/errscontract/rule_no_bare_command_error_test.go
- internal/qualitygate/semantic/schema_test.go
- internal/qualitygate/semantic/config_test.go
- internal/qualitygate/semantic/gatekeeper.go
- internal/qualitygate/rules/refs.go
- scripts/ci-quality-summary-publish.js
- internal/qualitygate/facts/schema_test.go
- internal/qualitygate/semantic/view_test.go
- internal/qualitygate/semantic/view.go
- internal/qualitygate/semantic/client_test.go
- internal/qualitygate/semantic/scope.go
- scripts/ci-quality-summary-publish.test.js
- internal/qualitygate/semantic/client.go
- internal/qualitygate/skillscan/harvest.go
- lint/errscontract/scan.go
- internal/qualitygate/rules/errorfacts_test.go
- internal/qualitygate/rules/naming_test.go
- internal/qualitygate/rules/errorfacts.go
- internal/qualitygate/rules/refs_test.go
- internal/qualitygate/facts/schema.go
- internal/qualitygate/semantic/ark_live_test.go
- internal/qualitygate/rules/dryrun_test.go
- lint/errscontract/rule_no_bare_command_error.go
liangshuo-1
approved these changes
Jun 17, 2026
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
3 participants
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.
Summary
This PR adds CI quality gates for machine-readable CLI contracts, covering command and flag naming, skill command references, dry-run examples, default output facts, and command-boundary error contracts. CI now runs deterministic checks, uploads base/head-bound facts artifacts, and lets semantic review consume only verified facts in observe-first mode by default.
Changes
tools/quality-gateandinternal/qualitygateto collect command, skill, error, output, and example facts, then run deterministic quality-gate checks.quality-gate/facts.jsonwith policy and waiver rules to produce observations or blockers.workflow_run-based PR Quality Summary publishing.Test Plan
make unit-testmake script-testmake quality-gatego vet ./...gofmt -l .produces no outputgo mod tidydoes not changego.modorgo.sumRelated Issues
Summary by CodeRabbit
New Features
Tests
Chores