feat(qualitygate): enforce six-layer package dependency boundaries#2045
Open
sang-neo03 wants to merge 12 commits into
Open
feat(qualitygate): enforce six-layer package dependency boundaries#2045sang-neo03 wants to merge 12 commits into
sang-neo03 wants to merge 12 commits into
Conversation
github-actions
Bot
added
the
size/S
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Go package-layering validation with seeded migration exceptions, deterministic graph evaluation, a Git-based ratchet preventing new denied edges, CI integration, shell tests, and removal of the circular-dependency audit step. ChangesLayering quality gate
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI lint
participant check-layering-ratchet.sh
participant layering-edges.txt
participant Git base
CI lint->>check-layering-ratchet.sh: pass QUALITY_GATE_CHANGED_FROM
check-layering-ratchet.sh->>layering-edges.txt: parse and hash current keys
check-layering-ratchet.sh->>Git base: read base ratchet keys
Git base-->>check-layering-ratchet.sh: return committed keys
check-layering-ratchet.sh-->>CI lint: pass or fail on new keys
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@217f4e55670f78d57c781aab9d456a5e27f32dd9🧩 Skill updatenpx skills add larksuite/cli#refactor/package-topology -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2045 +/- ##
==========================================
+ Coverage 75.13% 75.14% +0.01%
==========================================
Files 905 911 +6
Lines 96130 96322 +192
==========================================
+ Hits 72224 72385 +161
- Misses 18348 18370 +22
- Partials 5558 5567 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add a data-driven architecture layering test that builds the full import graph (go list -json -tags authsidecar) and evaluates six rules: - extension must not depend on internal (transitive; keeps it extractable as a standalone SDK module) - events must not depend on shortcuts (transitive) - shortcuts must not directly import auth/keychain/credential/client/vfs (direct; must go through the shortcuts/common RuntimeContext gate) - cmd subpackages must not import shortcuts (assembly point + cmd/auth only) - errs must stay a leaf - internal must not depend on cmd/shortcuts/events Pre-existing violations are seeded into layering-edges.txt (37 rows). The test rejects any unseeded violation (new debt) and any stale row (removed debt), and CI locks the effective row count to only ever decrease. Removes the tautological circular-dependency check from arch-audit.yml, since Go already forbids import cycles at compile time.
sang-neo03
force-pushed
the
refactor/package-topology
branch
from
July 24, 2026 07:18
37f4fb7 to
77cf968
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@internal/qualitygate/deptest/layering_test.go`:
- Around line 443-445: Update the parts parsing logic around the loop that trims
each field to reject whitespace-padded exception fields instead of mutating
them: compare each field with its trimmed form, return or report the existing
validation error when they differ, and only retain unchanged fields. Add a test
covering an exception row with padded input and assert it is rejected.
🪄 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 Plus
Run ID: 6b9d69e8-2cbf-4b9c-b4b3-857f6a69aca3
📒 Files selected for processing (4)
.github/workflows/arch-audit.yml.github/workflows/ci.ymlinternal/qualitygate/deptest/layering-edges.txtinternal/qualitygate/deptest/layering_test.go
💤 Files with no reviewable changes (1)
- .github/workflows/arch-audit.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- internal/qualitygate/deptest/layering-edges.txt
github-actions
Bot
added
size/L
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 `@scripts/check-layering-ratchet.sh`:
- Around line 16-17: Update the bootstrap defaults in the layering ratchet
script to reflect all 39 seeded edges: change the expected count and replace the
canonical hash with the hash calculated from the actual 39-edge snapshot. Also
update the associated bootstrap test expectations so the snapshot validation
accepts the seeded baseline.
🪄 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 Plus
Run ID: c674911d-4bb3-4501-86a3-2fb7a4d4af01
📒 Files selected for processing (7)
.github/workflows/ci.ymlMakefileinternal/qualitygate/deptest/layering-edges.txtinternal/qualitygate/deptest/layering_test.goscripts/check-layering-ratchet.shscripts/check-layering-ratchet.test.shscripts/ci-workflow.test.sh
💤 Files with no reviewable changes (1)
- internal/qualitygate/deptest/layering-edges.txt
Check all seven published GOOS and GOARCH combinations, and keep go list diagnostics separate from its JSON output for cold caches.\n\nMake the bootstrap snapshot immutable in CI, propagate shell failures explicitly, isolate sourced execution, and add deterministic regression tests for each contract.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check-layering-ratchet.sh (1)
23-31: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winWhitespace-padded fields are silently accepted instead of rejected.
Each field is trimmed via
trim()before validation, so a row with leading/trailing whitespace around otherwise-valid content passes silently rather than being flagged as malformed — the same issue previously raised for the Go-sideparseLayeringEdgesparser. This can let subtly malformed rows through unnoticed.🛡️ Proposed fix: reject rather than silently trim
from = trim($1) denied = trim($2) owner = trim($3) reason = trim($4) added_at = trim($5) - if (from == "" || denied == "" || owner == "" || reason == "" || added_at !~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/) { + if (from != $1 || denied != $2 || owner != $3 || reason != $4 || added_at != $5 || \ + from == "" || denied == "" || owner == "" || reason == "" || added_at !~ /^[0-9]{4}-[0-9]{2}-[0-9]{2}$/) { printf "Malformed layering ratchet row at %s:%d: fields must be non-empty and added_at must use YYYY-MM-DD.\n", FILENAME, FNR > "/dev/stderr" exit 2 }🤖 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/check-layering-ratchet.sh` around lines 23 - 31, Update the row validation in the AWK parsing block of check-layering-ratchet.sh to detect and reject leading or trailing whitespace in every raw field before or alongside trimming. Preserve the existing non-empty checks and YYYY-MM-DD validation, and report whitespace-padded rows as malformed rather than accepting the trimmed values.
🧹 Nitpick comments (1)
scripts/check-layering-ratchet.test.sh (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a shellcheck source directive to silence SC1090.
$scriptis computed dynamically, so shellcheck can't resolve it statically.+# shellcheck source=./check-layering-ratchet.sh source "$script"🤖 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/check-layering-ratchet.test.sh` at line 11, Add a ShellCheck source directive immediately before the dynamic `source "$script"` statement to suppress SC1090 for this intentionally computed path, without changing the script-loading behavior.Source: Linters/SAST tools
🤖 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/deptest/layering_test.go`:
- Around line 119-129: Update layeringBuildTargets and its surrounding
configuration to derive from or mirror the authoritative release target matrix
defined by goreleaser and the release workflow, including all published
GOOS/GOARCH combinations. Keep the layering test aligned with those release
targets so future matrix changes cannot leave this list stale.
---
Outside diff comments:
In `@scripts/check-layering-ratchet.sh`:
- Around line 23-31: Update the row validation in the AWK parsing block of
check-layering-ratchet.sh to detect and reject leading or trailing whitespace in
every raw field before or alongside trimming. Preserve the existing non-empty
checks and YYYY-MM-DD validation, and report whitespace-padded rows as malformed
rather than accepting the trimmed values.
---
Nitpick comments:
In `@scripts/check-layering-ratchet.test.sh`:
- Line 11: Add a ShellCheck source directive immediately before the dynamic
`source "$script"` statement to suppress SC1090 for this intentionally computed
path, without changing the script-loading behavior.
🪄 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 Plus
Run ID: 9d3bc06e-4a9d-4b3a-a937-6bb5070c615f
📒 Files selected for processing (4)
internal/qualitygate/deptest/layering_test.goscripts/check-layering-ratchet.shscripts/check-layering-ratchet.test.shscripts/ci-workflow.test.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci-workflow.test.sh
sang-neo03
force-pushed
the
refactor/package-topology
branch
from
July 24, 2026 09:06
37f0a6c to
e29119a
Compare
Layering edge parsing and graph coverage: - Reject whitespace-padded exception fields instead of silently trimming them, so a padded row is a malformed row rather than a coerced identity; add a padded-field parse test. - Fail loud when any release target/tag combination lists zero packages, which would otherwise let the layering graph silently under-cover. - Document the build-tag scope (demo tags excluded), the SkipFrom substring semantics, and the toolchain-derived support set behind the drift check. GoReleaser drift checks: - Reject custom build commands and per-target overrides as unsupported. - Detect --tags in addition to -tags when rejecting release build tags. - Reject any GO* build environment variable (except CGO_ENABLED=0) through a single default branch instead of an explicit allowlist. - Validate the GoReleaser global env block, and make the go-list stderr test table-driven across the default and authsidecar graphs.
sang-neo03
force-pushed
the
refactor/package-topology
branch
from
July 24, 2026 09:21
e29119a to
abe0d09
Compare
The extension rule skipped any package whose import path contained "/examples/", which let the gate miss two things: a directory named examples anywhere under extension escaped the rule outright, and the sanctioned demos were exempt from every denial rather than only from the internal packages they inherit through cmd. - Drop SkipFrom (and containsAny) so no rule can exempt by directory name. - Exempt the two wrapper-main demos from extension-zero-internal by exact import path. Their cmd import is the pattern they exist to demonstrate, and seeding those edges instead would wedge the ratchet: the edges track cmd's transitive set, so a new internal package under cmd would demand a new row that check-layering-ratchet.sh refuses by design. - Add examples-surface-only: demos may consume cmd and extension/platform but must not directly import internal or shortcuts. Zero violations today. layering-edges.txt stays at 39 rows, so the ratchet bootstrap snapshot still matches.
examples-surface-only promised that demos may consume only the assembled CLI and the public plugin SDK, but it enforced two denied prefixes instead, so every tree nobody thought to deny was permitted. A demo importing `events`, `errs` or a `cmd` subpackage passed the gate, and because extension-zero-internal exempts these packages from the transitive check, nothing examined what those imports dragged in either. The exemption was therefore unbounded in what it covered, the same defect as the directory-name skip it replaced. - Add Rule.AllowedRepoDeps, which inverts the check: any dependency inside this module that is not listed is a violation. Standard library and third-party packages, including same-organisation modules that are not this one, stay outside the rule. - Pin examples-surface-only to exactly `cmd` and `extension/platform`, so the rule name matches what it enforces and the inherited chain stays bounded by a direct surface of two packages. - Cover the reproducers as contract cases: other repository trees, `cmd` subpackages, other `extension` subtrees, and the module root are rejected, while the two allowed packages plus non-module imports are not. layering-edges.txt stays at 39 rows; the demos already import only the two allowed packages.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds machine enforcement for six package dependency layers. This PR installs enforcement only; no business packages are moved, and follow-up changes can remove the registered legacy edges.
Changes
Add a data-driven layering test in
internal/qualitygate/deptest/layering_test.go. It runsgo list -json ./...twice for each of the seven published Linux, Darwin, and WindowsGOOS/GOARCHcombinations - once untagged and once with-tags authsidecar- and unions the resulting non-test production dependency graphs (14 invocations). The union matters:extension/credential/sidecarand its 8 denied edges only exist under the tag. Standard error remains separate from JSON output so cold-cache module download messages cannot corrupt decoding. The test reads only.Importsand.Deps, so test-only imports stay outside this phase.Enforce seven rules:
extension/must not depend oninternal/**; the two wrapper-main demos (examples/audit-observer,examples/readonly-policy) are exempt by exact import path, because theircmdimport is the pattern they exist to demonstrate and every internal package they reach is inherited throughcmdextension/platform/examples/may directly import onlycmdandextension/platformfrom this module; every other repository package is rejected, while standard library and third-party imports stay outside the ruleevents/must not depend onshortcuts/**shortcuts/must use theshortcuts/commonruntime gate instead of importing sensitive runtime packages;shortcuts/commonandshortcuts/apps/gitcredare exemptcmdassembly root andcmd/authmay importshortcuts/**errsmust remain a repository leafinternal/must not importcmd,shortcuts, orevents; themanifest-exportcommand collector is exemptExempt packages by exact import path only. An earlier revision skipped any package whose path contained
/examples/, which meant a directory namedexamplesanywhere underextension/escaped the rule outright, and the sanctioned demos were exempt from every denial rather than only from the internal packages they inherit throughcmd. That substring mechanism is gone, andexamples-surface-onlynow covers the demo tree as an allowlist rather than a few denied prefixes: because the transitive rule no longer inspects these packages, pinning their direct surface to two packages is what keeps the inherited chain bounded. Seeding those inherited edges instead would wedge the ratchet: they trackcmd's transitive set, so a newinternalpackage undercmdwould demand a new row thatcheck-layering-ratchet.shrefuses by design.Register 39 existing
(from, denied)edges inlayering-edges.txt. The Go test rejects new unregistered edges and stale rows. CI compares the current key set with the base revision, so keys may only be removed; equal-count replacement also fails. The first-run bootstrap is locked to the exact approved 39-edge snapshot.Add contract tests for unchanged metadata, deletion, addition, equal-count replacement, malformed input, missing files, duplicate keys, bootstrap replacement, invalid base revisions, conditional Shell execution, and separated
go listoutput streams.Remove the circular-dependency report step from
arch-audit.yml; Go already rejects import cycles during compilation.Test Plan
make script-testmake unit-testgo vet ./...gofmt -l .go mod tidyleavesgo.modandgo.sumunchangedgolangci-lintreports zero issuesGOMODCACHEandGOCACHErun passes all seven package graph targetseventsimport fails withfrom,denied, andrulediagnosticsinternalimport insideexamples/audit-observerfailsexamples-surface-onlyevents,errsand acmdsubpackage imported fromexamples/audit-observerall failexamples-surface-onlyexamples/directory inextension/failsextension-zero-internalmake examples-buildstill builds both wrapper-main demosRelated Issues
Summary by CodeRabbit
Quality Improvements
CI / Automation
script-testtarget to include layering ratchet checks.