refactor: enforce package boundaries and eliminate recorded dependency debt by sang-neo03 · Pull Request #2055 · larksuite/cli

sang-neo03 · 2026-07-25T09:59:39Z

Summary

Enforces the repository's package dependency boundaries in production and test files, removes every registered layering exception, and separates the former internal/core package by responsibility. Phase two and phase three remain in one PR based on main; functional behavior, network requests, error text, hints, identity metadata, and command output are unchanged.

The exception registry reaches main empty and stays empty for both graphs: the 39 production edges are fixed rather than registered, and the 16 test-file edges the second review round surfaced are resolved by policy (Rule.TestExempt) rather than by rows.

Changes

Dependency enforcement and phase two cleanup

Add the data-driven dependency gate in internal/qualitygate/deptest, covering the published release targets and the authsidecar build graph.
Register the 39 pre-existing violations, then remove 21 edges by separating extension-safe environment names, moving message conversion below shortcuts, extracting authentication logging, and splitting binding responsibilities.
Add the CI ratchet that permits removal of registered (from, denied) keys and rejects additions or replacements. added_at is immutable on an edge the base already carries, so an accepted debt cannot be re-dated without changing an import; owner and reason stay editable, since a handoff has to remain possible and the change is legible in the diff.
Check the imports test files bring in. go list reports them in TestImports and XTestImports, which the graph did not read, so a denied dependency reached the tree through any _test.go file unchecked — ten packages were already through that gap. TestPackageLayering now walks a second graph derived from those fields, and Rule.TestExempt records where the two views legitimately differ: a shortcut's test may name the credential, auth and filesystem packages the runtime it constructs is assembled from; internal/keychain and internal/client stay denied, because a test needs neither to build a RuntimeContext. Direction is denied in both views — a test may reach down for scaffolding, never up.

Remove the final 18 layering exceptions

Route token scope lookup and comparison through shortcuts/common while preserving every caller's existing error, warning, hint, identity, and missing-scope behavior.
Route request options, network error wrapping, response filenames, and validated output-directory creation through permitted lower-level entry points.
Move Spark application state into internal/sparkstore and keychain-typed helpers into shortcuts/apps/gitcred.
Reduce internal/qualitygate/deptest/layering-edges.txt to its header, with zero approved exceptions.

Set the bootstrap snapshot to the empty registry

origin/main does not contain layering-edges.txt, so this combined PR necessarily uses the bootstrap branch of check-layering-ratchet.sh.
The registry first reaches main with zero exception keys. The approved bootstrap count therefore changes from 18 to 0, and its hash changes to the SHA-256 value for an empty key file.
The baseline remains hardcoded and can only be lowered by this reviewed diff; the registry file itself remains present with its header.

Split internal/core

Extract internal/risk, repository-root brand, internal/workspace, internal/secret, and internal/identity, then rename the remaining configuration package to internal/config.
Keep the public extension/credential brand surface source-compatible through type and constant aliases while sharing the repository-root implementation.
Delete the duplicate brand parser and parity test, remove the internal/core directory, and update the source-layout entry in AGENTS.md.
Remove four internal legacy config loaders that had no production or test callers. Their old file location was part of the existing dead-code baseline; deleting them prevents a pure package move from appearing as newly introduced dead code.
Update the endpoint contract scanner's canonical resolver source from internal/core/types.go to brand/brand.go. The exemption remains limited to the receiverless ResolveEndpoints function body; literals elsewhere in that file are still rejected.

Fixes from review

Restore the empty-content guard above the merge_forward dispatch in convert_lib. The extraction moved it down into internal/imcontent, which the shortcut-side merge_forward converter never consults, so a merge_forward message with an empty body.content stopped converting to "": with a prefetched page it rendered a full <forwarded_messages> subtree, and without one it issued an extra GET /open-apis/im/v1/messages/{id}. This was the one place where the "output and requests unchanged" claim did not hold; it holds again, with a regression test for each half.
Report a stale ratchet base as a stale base. The bootstrap branch is selected by "the base revision has no registry", which after this merges also describes every branch forked before it — those PRs were told their bootstrap snapshot was wrong, and the obvious response, editing the approved baseline, is the opposite of the intended one. The failure now names both situations, the action each takes, and the keys it found.
Keep both layering walks out of dot directories. go list ./... never builds them, so requiring a configuration to have compiled .cache/**.go failed the suite for any developer with build scratch in the tree.
Keep internal/outputdir. Inlining it into shortcuts/common passes the layering gate, which exempts that package as the runtime gate, and fails the depguard rule shortcuts-no-vfs, which does not. The forwarder is load-bearing and now says so in both files.
Drop the forwarders in convert_lib that only a test kept alive, and move the tests to the packages that own the code: internal/imcontent 82.0% → 89.1%, internal/sparkstore 72.6% → 94.5%, internal/outputdir 0% → 100%.
internal/output's frozen-oracle test imported shortcuts/common to run its fixtures through RuntimeContext.Out* — the upward dependency internal-no-upper forbids, invisible while the gate read production imports only. The Emitter half stays with the fixtures; the wiring half moves to the layer that owns those methods, and covers OutFormatRaw and the notice provider, which the oracle reached and no other test did.
Two merge-forwards from main carried semantic conflicts with no textual overlap: envvars.CliAgentName, unexported here after the surface trim, gained a cross-package reader in feat: propagate invocation metadata #2097 and is exported again; contact_search_bot_test.go from feat(contact): add bot search shortcut #2083 built its fixture from core.CliConfig, which stops the merged tree compiling and fails fast-gate before anything else runs.

Test Plan

Related Issues

Supersedes feat(qualitygate): enforce six-layer package dependency boundaries #2045

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.

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.

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.

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.

Removing the internal/core dependency left each credential provider with its own copy of the brand rule. Two identical five-line functions mean the brand set can grow in one provider and silently not in the other, with nothing to catch it at build time. Move the rule next to the Brand constants as credential.ParseBrand and have both providers call it. Same behaviour, one definition.

Once events switched to internal/imcontent directly, the convert_lib wrapper for ConvertInteractiveEventContent had no callers left. It is newly unreachable code, which the CI dead-code gate rejects because it only tolerates entries that already exist on the base branch.

Extracting the logger from keychain replaced an injected package variable with per-call construction, which regressed two things. Keychain errors went to the wrong directory. cmdutil used to inject core.GetRuntimeDir into keychain, so every auth diagnostic landed in the workspace-aware log. keychain now built its logger with empty Options, falling back to the pre-workspace ~/.lark-cli path while internal/auth kept passing core.GetRuntimeDir. Inside a workspace the two halves of one investigation split across two directories, and LARKSUITE_CLI_LOG_DIR masks it whenever that override is set. Each call also built a fresh logger. The sync.Once guarding file creation is per instance, so every logged line reopened the file — never closed — and re-ran the week-old-log prune. wrapError fires on every keychain operation, and a locked keychain is exactly the failure this log exists to diagnose. Install one logger while the command factory is built, which is the only place that knows the workspace-aware directory: authlog cannot resolve it itself because internal/core imports internal/keychain, which imports authlog. Both callers now read that shared instance, so there is one file handle and one prune per process. Tests pin the singleton and the install-wins behaviour. The process-wide variable is a stopgap: the internal/core split can hand the runtime directory to authlog directly and remove the indirection.

coderabbitai · 2026-07-25T09:59:47Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR adds dependency-layering enforcement, public environment-name constants, centralized message conversion and authentication logging, and internal package namespace migrations. Callers, tests, workflows, and scripts are updated accordingly.

Changes

Layering quality gate

Layer / File(s)	Summary
Dependency layering enforcement `.github/workflows/`, `Makefile`, `internal/qualitygate/deptest/`, `scripts/check-layering-ratchet*`	Adds dependency-layering tests, seeded edges, ratchet validation, CI enforcement, and script coverage; removes circular dependency reporting from the architecture audit.

Shared environment and content contracts

Layer / File(s)	Summary
Environment names and credential consumers `envnames/`, `extension/credential/`, `extension/transport/`, `cmd/`, `sidecar/`, `main_noauthsidecar`, `internal/envvars/*`	Exports CLI environment-name constants, adds credential brand parsing, and migrates production code and tests from internal environment constants.
Message content conversion `internal/imcontent/`, `shortcuts/im/convert_lib/`, `events/im/message_receive.go`	Centralizes content conversion, mention handling, timestamps, post extraction, and merge-forward parsing, then delegates shortcut and event conversion through `imcontent`.

Authentication logging and package boundaries

Layer / File(s)	Summary
Shared authentication logger `internal/authlog/`, `internal/auth/`, `internal/cmdutil/`, `internal/keychain/`	Adds configurable shared file logging and routes authentication responses and errors through injected logger instances.
Internal namespace migration `cmd/config/binder.go`, `internal/openclawbind/`, `internal/secaudit/`, `internal/transport/*`	Renames internal binding and security-audit packages and updates OpenClaw, transport, and secure-path callers.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

larksuite/cli#235: Modifies the same authentication response logging call sites and logging plumbing.
larksuite/cli#515: Introduces the OpenClaw binding and secret-resolution implementation used by the binder migration.
larksuite/cli#2045: Directly overlaps with the layering ratchet, CI checks, seed data, and workflow changes.

Suggested labels: size/XL

Suggested reviewers: liangshuo-1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.31% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title matches the PR’s main focus on enforcing package boundaries and repaying dependency debt.
Description check	✅ Passed	The description includes the required Summary, Changes, Test Plan, and Related Issues sections with substantive details.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/package-debt-phase2

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…stry The approved bootstrap snapshot records the registry size at the moment the file first reaches the target branch, and it is the only check that runs in that situation. This change now carries both the registry and the first round of cleanup, so the file lands on main holding 18 edges rather than the 39 it was pinned to. Update the count and hash to match, and state in a comment why the baseline stays hardcoded: a value CI could supply would let anyone raise the approved size without the change appearing in a diff.

main gained risk-control host signals, which changed the cachedHttpClientFunc and cachedLarkClientFunc signatures and rewrote the proxy-warning test around TestFactory. The only conflict was that test's import block: this branch moved the shared environment variable names out of internal/envvars into envnames, while main still imported the old package. Keep envnames for the five constants that moved and add internal/core for the config types the rewritten test now builds. internal/envvars is no longer needed here; its remaining constants are the internal-only ones.

coderabbitai

Actionable comments posted: 4

🤖 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/authlog/authlog.go`:
- Around line 95-122: Update defaultRuntimeDir and Logger.logDir to validate
LARKSUITE_CLI_CONFIG_DIR and LARKSUITE_CLI_LOG_DIR with validate.SafeEnvDirPath,
and return a typed validation error when either explicit override is invalid
instead of falling back or redirecting. Adjust internal/authlog/authlog_test.go
lines 27-40 to assert the validation failure rather than the fallback directory
behavior.
- Around line 151-155: Update the argument formatting logic in authlog to avoid
persisting raw os.Args: retain command names while redacting sensitive flags and
their values, including app-secret, before joining and truncating the arguments
for the log. Ensure secrets are never emitted in the returned command string.

In `@internal/imcontent/helpers.go`:
- Around line 62-65: Update FormatTimestamp in internal/imcontent/helpers.go to
format timestamps at minute precision using the existing calendar date format,
preserving the HH:mm contract. Align the wrapper documentation or output
contract in shortcuts/im/convert_lib/helpers.go with this behavior, and add a
direct regression test covering the minute-precision result.

In `@sidecar/server-demo/handler_test.go`:
- Around line 423-428: The self-proxy validation in the run handlers must return
the prescribed typed validation/precondition error while preserving the
underlying cause, replacing the current fmt.Errorf path in both server-demo and
server-multi-tenant-demo main.go. Update TestRun_RejectsSelfProxy in
sidecar/server-demo/handler_test.go lines 423-428 and
sidecar/server-multi-tenant-demo/handler_test.go lines 419-424 to use
errs.ProblemOf and errors.As, asserting the typed error metadata and preserved
cause rather than only matching message text.

🪄 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: a5ee3fe3-c391-46b9-b4bd-4ef390b2df39

📥 Commits

Reviewing files that changed from the base of the PR and between a7865cd and 602f671.

📒 Files selected for processing (84)

.github/workflows/arch-audit.yml
.github/workflows/ci.yml
Makefile
cmd/config/binder.go
cmd/root_integration_test.go
cmd/startup_brand.go
envnames/envnames.go
events/im/message_receive.go
extension/credential/env/env.go
extension/credential/env/env_test.go
extension/credential/sidecar/provider.go
extension/credential/sidecar/provider_test.go
extension/credential/types.go
extension/transport/sidecar/interceptor.go
internal/auth/app_registration.go
internal/auth/auth_response_log.go
internal/auth/device_flow.go
internal/auth/device_flow_test.go
internal/auth/revoke.go
internal/auth/uat_client.go
internal/auth/verify.go
internal/auth/verify_test.go
internal/authlog/authlog.go
internal/authlog/authlog_test.go
internal/cmdutil/factory_default.go
internal/cmdutil/factory_default_test.go
internal/cmdutil/factory_proxy_warn_test.go
internal/cmdutil/factory_test.go
internal/credential/integration_test.go
internal/envvars/envvars.go
internal/envvars/read.go
internal/envvars/read_test.go
internal/imcontent/card.go
internal/imcontent/card_test.go
internal/imcontent/card_userdsl.go
internal/imcontent/card_userdsl_test.go
internal/imcontent/convert.go
internal/imcontent/helpers.go
internal/imcontent/media.go
internal/imcontent/misc.go
internal/imcontent/text.go
internal/imcontent/text_test.go
internal/keychain/auth_log.go
internal/keychain/auth_log_test.go
internal/keychain/keychain.go
internal/openclawbind/json_pointer.go
internal/openclawbind/json_pointer_test.go
internal/openclawbind/lark_channel.go
internal/openclawbind/lark_channel_test.go
internal/openclawbind/reader.go
internal/openclawbind/reader_test.go
internal/openclawbind/secret_resolve.go
internal/openclawbind/secret_resolve_exec.go
internal/openclawbind/secret_resolve_exec_test.go
internal/openclawbind/secret_resolve_file.go
internal/openclawbind/secret_resolve_file_test.go
internal/openclawbind/secret_resolve_test.go
internal/openclawbind/tilde.go
internal/openclawbind/tilde_test.go
internal/openclawbind/types.go
internal/openclawbind/types_test.go
internal/qualitygate/deptest/layering-edges.txt
internal/qualitygate/deptest/layering_test.go
internal/secaudit/audit.go
internal/secaudit/audit_test.go
internal/secaudit/audit_unix.go
internal/secaudit/audit_windows.go
internal/secaudit/audit_windows_test.go
internal/transport/config.go
internal/transport/tls_ca.go
main_noauthsidecar.go
main_noauthsidecar_test.go
scripts/check-layering-ratchet.sh
scripts/check-layering-ratchet.test.sh
scripts/ci-workflow.test.sh
shortcuts/im/convert_lib/content_convert.go
shortcuts/im/convert_lib/content_media_misc_test.go
shortcuts/im/convert_lib/helpers.go
shortcuts/im/convert_lib/merge.go
shortcuts/im/convert_lib/pure_adapters.go
sidecar/server-demo/handler_test.go
sidecar/server-demo/main.go
sidecar/server-multi-tenant-demo/handler_test.go
sidecar/server-multi-tenant-demo/main.go

💤 Files with no reviewable changes (4)

internal/keychain/auth_log.go
internal/envvars/envvars.go
internal/keychain/auth_log_test.go
.github/workflows/arch-audit.yml

coderabbitai · 2026-07-25T10:16:40Z

+	if len(strings.TrimLeft(timestamp, "+-")) >= 13 {
+		value /= 1000
+	}
+	return time.Unix(value, 0).Local().Format("2006-01-02 15:04:05")


🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve the minute-precision timestamp contract.

FormatTimestamp now returns HH:mm:ss, but the migrated shortcut contract is HH:mm; calendar and todo output therefore changes during a refactor intended to centralize conversion. Restore 2006-01-02 15:04 and add a direct regression test.

internal/imcontent/helpers.go#L62-L65: format the centralized timestamp at minute precision.

shortcuts/im/convert_lib/helpers.go#L30-L31: keep the wrapper’s documented output contract aligned with the centralized implementation.

As per coding guidelines, “When transcribing input or transforming requests, preserve values faithfully.”

📍 Affects 2 files

internal/imcontent/helpers.go#L62-L65 (this comment)

shortcuts/im/convert_lib/helpers.go#L30-L31

🤖 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/imcontent/helpers.go` around lines 62 - 65, Update FormatTimestamp in internal/imcontent/helpers.go to format timestamps at minute precision using the existing calendar date format, preserving the HH:mm contract. Align the wrapper documentation or output contract in shortcuts/im/convert_lib/helpers.go with this behavior, and add a direct regression test covering the minute-precision result.

Source: Coding guidelines

github-actions · 2026-07-25T10:22:53Z

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a72297b0267ae17118a476a12f2314ac41667cd8

🧩 Skill update

npx skills add larksuite/cli#refactor/package-debt-phase2 -y -g

codecov · 2026-07-25T10:24:32Z

Codecov Report

❌ Patch coverage is 76.77165% with 177 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.19%. Comparing base (87be09e) to head (a72297b).

Files with missing lines	Patch %	Lines
shortcuts/common/runner.go	28.12%	23 Missing ⚠️
internal/authlog/authlog.go	84.61%	13 Missing and 9 partials ⚠️
shortcuts/apps/gitcred/app_manager.go	0.00%	21 Missing ⚠️
cmd/config/init.go	41.17%	20 Missing ⚠️
cmd/config/bind.go	76.92%	5 Missing and 1 partial ⚠️
cmd/config/strict_mode.go	64.70%	4 Missing and 2 partials ⚠️
internal/credential/credential_provider.go	40.00%	6 Missing ⚠️
cmd/event/consume.go	28.57%	5 Missing ⚠️
internal/sparkstore/git_credential.go	78.26%	3 Missing and 2 partials ⚠️
cmd/config/default_as.go	0.00%	4 Missing ⚠️
... and 35 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2055      +/-   ##
==========================================
- Coverage   75.25%   75.19%   -0.06%     
==========================================
  Files         916      922       +6     
  Lines       97167    97253      +86     
==========================================
+ Hits        73121    73128       +7     
- Misses      18433    18502      +69     
- Partials     5613     5623      +10

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The internal/core split renamed or relocated every symbol these comments name, and the sweep missed six call-outs. Two had gone self-contradictory: internal/meta named the package internal/core while already qualifying the type as identity.Identity, and authlog attributed its runtime-directory indirection to a cycle through internal/core that the split removed. Restate authlog's reason for keeping the indirection instead of promising a follow-up: the factory-installed logger follows the detected workspace while the Shared() fallback stays on the pre-workspace directory, so resolving the directory inside the package would collapse that distinction. The domaincontract rule's README still pointed host literals at internal/core/types.go; the exemption moved to brand/brand.go with the resolver.

The split left the layout table naming only the renamed config package, so brand, workspace and identity — the three a caller reaches for most — had no entry, and nothing said where the remaining two went. brand earns a row of its own for a second reason: it sits at the repository root precisely so extension/ may import it. The note under the table carries what a path table cannot. The five siblings do not import each other, and that is the whole reason to ask for the narrow one: a caller that only wants a config directory no longer compiles keychain, i18n and validate along with it.

…-phase2 Two conflicts, both in the calendar rich-image preview URL and both the same shape: main fixed the default host while this branch renamed the brand type. Took main's fix and this branch's package. - shortcuts/calendar/description_rich_images.go: host stays the feishu.cn value #2079 corrected it to, with the brandpkg.Brand signature. - shortcuts/calendar/description_rich_images_test.go: the expected host fragment follows the same fix; the table already carried brandpkg.Brand from the automatic merge, so core.Brand* would not have compiled.

The union executed one `go list` per registered tag, so a file constrained by `foo && bar` was selected by neither `-tags foo` nor `-tags bar`. The coverage test only asked whether each tag name appeared in the registry, which both did, and the remedy it printed — "union the tag" — is what produced the gap: register the two tags separately and the file lands in no graph while every check reports covered. A probe file under events/ importing shortcuts/common, the exact shape events-no-shortcuts forbids, passed the whole suite that way. Derive the configurations instead. Every //go:build line is parsed with go/build/constraint, and each distinct expression contributes a tag set that satisfies it, so `foo && bar` yields `-tags bar,foo` without anyone registering anything. Platform terms stay free variables: layeringBuildTargets already varies GOOS and GOARCH, and -tags cannot set them. Cheapest set wins, which keeps a platform-only constraint from adding a configuration it does not need. Deriving the sets removes both hand-kept lists, including the exclusion map that carved out the sidecar demo tags — those now get a configuration like everything else. The replacement invariant is a file-level one the tag registry could not state: every Go file in this module must be compiled by at least one executed configuration, asked of the toolchain rather than re-derived from the model that produced the configurations. It immediately found a second blind spot with no custom tag in it at all: internal/riskcontrol/osmodel_other.go is constrained `!darwin && !windows && !linux`, and all seven release targets are one of those, so no configuration has ever compiled it. That file is recorded as out of scope with its reason checked — an entry naming a custom tag, or one a release target does compile, now fails. Nested modules are skipped, since `go list ./...` does not reach into lint/ and the rules are written against this module's import paths. Cost: the file-level check sweeps `go list` again because the rule graph merges packages across configurations and keeps only imports, so it can no longer say which configuration contributed which file. The package goes from roughly 16s to roughly 60s.

The internal/core split renamed the brand package and aliased the import as brandpkg, and the sweep that rewrote `brand.` also rewrote the word ending three sentences. One of them is user-visible: `apps --help` on Lark read "The "apps" feature is not yet supported on the lark brandpkg." The error path a few lines above was spelled without the trailing period and escaped the sweep, so the two surfaces disagreed. The brand-guard tests only exercised RunE, which --help bypasses, so nothing covered the sentence. Pin it whole: a substring check would still pass on a mangled tail. The other two are comments in internal/auth.

The constraint walk descended into nested modules while the file walk skipped them, and the two feed each other: constraints become the configurations the file walk is measured against. A compound tag under lint/ therefore added `-tags bar,foo` to this module's sweep — seven more `go list` runs selecting nothing here, and a failing configuration list — over a file no walk ever required to be selected. lint/ carries no custom tag today, so the divergence was latent rather than broken. One predicate now answers both walks, and a test pins it: a nested module is out of scope, a plain package directory is not, and the module root itself stays in.

ConvertBodyContent opened with `if ctx.RawContent == "" { return "" }` before the converters moved down to internal/imcontent. The guard went with them, and merge_forward is dispatched above that call — the shortcut-side converter expands the tree from the API rather than from body.content, so it never reaches imcontent's copy. A merge_forward item whose body.content is an empty string therefore stopped converting to "" the way every other message type does. With a prefetched page it renders a full <forwarded_messages> subtree; without one, and with a runtime in hand, it issues an inline GET /open-apis/im/v1/messages/{id} and prints "[Merged forward: fetch failed: ...]" when that fails. Both reach the formatted message output. FormatEventMessage carries no Runtime and no prefetch, so the event path kept falling through to imcontent and was unaffected. The guard belongs above the dispatch, where it was, and now also covers a nil context, which the original would have dereferenced. Two tests pin it: the prefetch path must still convert to "", and the runtime path must issue zero requests.

check-layering-ratchet.sh picks bootstrap or incremental mode by whether the base revision carries layering-edges.txt. Once the registry is on the target branch, every PR whose merge base predates that point still lands in bootstrap mode, so a branch that legitimately registers one exception was told its bootstrap differs from the approved 0-edge snapshot. The obvious response to that message — edit the approved baseline — is the opposite of what the gate wants. A gate-version marker cannot separate the two cases: the script and the registry land in the same commit, so "the base has neither" describes both the commit that introduces the gate and any branch that forked before it. The failure now names both situations, says what each one does, and lists the keys it found, so a developer on a stale base sees the row to fix and the rebase that will report it as a new key instead. added_at also becomes immutable on a key the base already carries. That date is the ratchet's clock: it records when the debt was accepted, and moving it makes an old exception look fresh, or a fresh one grandfathered, without touching a single import. owner and reason stay editable — their change is legible in the diff, and locking them would leave no way to hand an exception over, since the gate has no override short of deleting a row the dependency still needs.

skipLayeringScopeDir named .git explicitly and skipped every "_" prefix, but the go command ignores "." and "_" alike, so `go list ./...` never offers a dot directory's files to any configuration. The walk descended into them anyway, and TestLayeringBuildConfigsSelectEveryFile then demanded a configuration that had compiled them: a gitignored .cache/ holding one Go probe file fails the suite with "is compiled by none of the 28 executed configurations". CI checks out clean, so this only ever bit a developer with build scratch in the tree. The predicate now answers the scope its own comment claims — what `go list ./...` builds — and the module root stays in scope whatever it is called, so a checkout under a directory the rules would otherwise reject does not empty both walks.

internal/outputdir had one importer: a shortcuts/common function that forwarded to it and did nothing else. shortcuts/common is the runtime gate that shortcuts-runtime-gate exempts, so it already holds vfs and validate, and the package below it held nothing the gate does not. EnsureOutputDir is the whole implementation again, and gains the first tests it has had — four callers, no coverage until now: a relative path resolved inside the working directory, one that climbs out and must be rejected before anything is created, and the absolute path its doc comment promises to accept. convert_lib kept four forwarders into internal/imcontent. ResolveMentionKeys, formatTimestamp and extractPostBlocksText had no caller but a test, and forwarding ParseJSONObject only gave one function two entry points; its two real callers in resource_extract.go now say imcontent.ParseJSONObject. BuildMentionKeyMap stays, because shortcuts/event builds a ConvertContext through this package and should not have to reach past it. The five helper tests move to internal/imcontent, where the code they cover lives, so removing a forwarder no longer removes coverage. Two files that arrived without tests of their own get them: the imcontent dispatch — including the invariant a converter table cannot state, that a registered type must never be answered by the "[type]" placeholder — and sparkstore's AppStorage adapter, where ListAppIDs decodes escaped directory names and must report an absent root as zero apps rather than an error. Own-package coverage: imcontent 82.0% -> 89.1%, sparkstore 72.6% -> 94.5%.

…-phase2 One conflict, in internal/envvars/read_test.go, and it stands for a real disagreement rather than two edits to the same line. This branch unexported CliAgentName into a package-private agentNameEnv while trimming the envvars surface (8ba2431): at that point read.go and its test were the constant's only readers. main then landed #2097, whose internal/cmdutil/secheader_test.go sets the variable through envvars.CliAgentName — a cross-package reader again, so the constant earns its export back. Keeping it private would have meant spelling "LARKSUITE_CLI_AGENT_NAME" a second time in cmdutil, which is what the constant exists to prevent. So: CliAgentName is restored in envvars.go, read.go reads it instead of the private duplicate, and read_test.go is taken from main — that keeps #2097's de-branding of the fixtures ("sample-agent" in place of the two agent names the test used to hardcode), which this branch had no stake in. Verified on the merge result: go build ./..., go vet ./... and go test -count=1 over ./internal/... ./shortcuts/... ./events/... ./cmd/... ./extension/... are clean, as are the layering ratchet and ci-workflow script suites.

The layering graph was built from Imports and Deps only. `go list` keeps a package's test dependencies in two other fields — TestImports for the in-package test files, XTestImports for the external test package — and listedPackage did not declare either, so every denied dependency reached through a _test.go file went unreported. Ten packages were already through the gap: shortcuts/mail's tests import internal/auth, internal/vfs and internal/vfs/localfileio, and the rule that denies exactly those to shortcuts stayed green. TestLayeringBuildConfigsSelectEveryFile made it worse than a plain omission. It counts TestGoFiles and XTestGoFiles as selected, on the stated ground that "an import edge only reaches the rules through a selected file" — so the check that exists to prove nothing is unscanned was vouching for files the rules never read. TestPackageLayering now walks a second graph, testDependencyView, built from those two lists: direct imports for a Direct rule, and for a Transitive one the closure through each test import's production deps. A package's own import path is dropped, because `package foo_test` always imports foo and errs-leaf denies this module wholesale — counting that would fail the leaf on the test that tests it. The two graphs need different answers, so Rule gains TestExempt. A shortcut's test builds the runtime the shortcut is handed at run time, which means naming the credential, auth and filesystem packages that runtime is assembled from; denying those in tests moves no production import and would only park ten packages in the exception registry for writing ordinary tests. keychain and client stay denied in tests too: a test needs neither to construct a RuntimeContext, and reaching for them means it is talking to the real keyring or issuing real requests. Direction stays denied everywhere — a test may reach down for scaffolding, never up. That last part left one real violation, and it was an inversion rather than scaffolding: internal/output's frozen-oracle test imported shortcuts/common to run the same fixtures through RuntimeContext.Out*. The Emitter half stays where the fixtures are; the wiring half moves to the layer that owns those methods, as shortcuts/common/runner_emitter_wiring_test.go — each Out* has to hand the Emitter the option its name promises, which the bytes show (Raw decides whether `<p>a&b</p>` survives). It also covers OutFormatRaw, which the oracle was the only test to reach. Statement coverage is unchanged in both packages, 83.5% and 72.6%. Verified by probe, both buckets: an XTest-only and an in-package-test-only import of a denied package under shortcuts/mail each fail the gate now, reported with in=test files, and passed it before this change.

4c22015 inlined internal/outputdir into shortcuts/common on the grounds that the package below held nothing the runtime gate does not. It held one thing: the internal/vfs import. The depguard rule shortcuts-no-vfs denies vfs to every file under shortcuts/ and grants no exemption, while the layering rule shortcuts-runtime-gate exempts shortcuts/common as the runtime gate. Two gates, different answers about the same package — so the inline built, passed the layering gate, and failed the lint job on the one line it added: shortcuts/common/output_dir.go:10:2: import '.../internal/vfs' is not allowed from list 'shortcuts-no-vfs' (depguard) The forwarder is restored with the reason written down in both files, so the next reader does not measure the hop against the layering rule alone and reach the same wrong conclusion. The tests stay, split where each belongs: internal/outputdir owns the behaviour it implements — relative paths resolved inside the working directory, an escaping path rejected before anything is created, an absolute path accepted — and reaches 100% of its statements, which is more than the 0% that made the hop look dead in the first place. shortcuts/common keeps one test for the only way a forwarder this thin can fail, by not being called. The gap that let this reach CI: the verification for 4c22015 ran the layering gate and the repository's own lintcheck, neither of which is golangci-lint. `golangci-lint run --new-from-rev` — the command the lint job actually runs — now reports 0 issues for this branch.

ae56d30 added the test-import graph and covered it with fixtures handed straight to testDependencyView and evaluateLayeringTestRule. That left the wiring itself uncovered: deleting either merge line in goListPackageGraph, or the test-view evaluation in the gate's aggregation, kept the whole deptest suite at exit 0. Three independent mutations, three green runs — the reverse check for this gate lived in a shell session instead of in CI. Two tests close it, one per half of the wiring. The aggregation moves into layeringViolationsByRule, so consulting both graphs is now code a test can reach. TestLayeringViolationsByRuleReportsTestOnlyEdges runs the real rule set over a package whose production imports are clean and whose denied dependencies exist only in TestImports and XTestImports; both have to come back, grouped under their rule and marked TestOnly. It doubles as the statement of what stays denied to tests — keychain and client, the two the shortcuts TestExempt list leaves out. TestGoListGraphCarriesBothTestImportKinds covers what no fixture can: that the two fields survive `go list -json` and the per-configuration merge. Its expectation is derived from the tree rather than pinned to a package that happens to import something today — whichever kinds of test file the module contains, in-package or external, must appear in the graph. Verified by mutation: each of the three deletions now fails, and named for the reader — "goListPackageGraph is dropping the field" for the merges, "the gate is not consulting the test view" for the aggregation.

…-phase2 No textual conflict this time, and that is the problem it hides: a575a8b added shortcuts/contact/contact_search_bot_test.go, which builds its fixture config from core.CliConfig and core.BrandFeishu. This branch split internal/core, so the merged tree stops compiling — `go vet ./...` reports no module provides internal/core, which fails fast-gate before anything else runs and skips ten checks behind it. Git had no way to see it: neither side touched a line the other did. The new test now says configpkg.CliConfig and brand.Feishu, matching its neighbour contact_search_user_test.go in the same package, which was ported when the split landed. Nothing else in the merge referred to the removed package; the two remaining mentions of internal/core in the tree are a comment in internal/authlog and a fixture import path inside the layering rule engine's own tests. Verified on the merge result: go build ./..., go vet ./..., go test -count=1 over ./internal/... ./shortcuts/... ./events/... ./cmd/... ./extension/..., and `golangci-lint run --new-from-rev` at 0 issues.

TestGoListGraphCarriesBothTestImportKinds derived its expectation from this repository: it scanned the tree for in-package and external test files, then required the graph to report the kinds it found. That reads as thorough and fails open — the day the last external test package is deleted or moved, the XTestImports half stops asserting anything and the suite still passes. It now builds a module for the purpose: a subject package that imports one package from an in-package test file and another from its external test package, so neither field can be satisfied by the file that satisfies the other. Same `go list` and same merge path, deterministic input, and 0.2s instead of 35s because the sweep runs over four packages rather than the whole repository. Mutation-checked again: dropping either merge line fails with the field named. Three other gaps from the same review, all of them tests for behaviour that is already correct: - shortcuts/common asserted every EmitOptions field the Out* methods forward except the notice provider. A dropped assignment there stays valid JSON, so nothing would have failed — it would only show up as users no longer being warned their token is about to expire. - internal/outputdir and its shortcuts/common forwarder covered success, rejection and the absolute path, but never a filesystem failure. Both now create a regular file where a parent directory has to be and require the error to come back, rather than a success for a directory that does not exist. - internal/cmdutil explained the authlog install order in terms of internal/core, which this branch split. The cycle it describes is still real — keychain imports authlog — so the comment now names that instead of a package the reader cannot find. Left alone deliberately: splitting layering_test.go. It is 2770 lines holding the rules, the go list plumbing, the graph, the registry and their tests, and it should be several files in the same package. That is a pure file move, and doing it inside a review round about this PR's size would bury the changes above in relocation noise. Better as the first commit after this merges.

sang-neo03 added 19 commits July 24, 2026 15:18

fix(qualitygate): harden layering ratchet enforcement

acd50f2

test(qualitygate): tolerate coverage helper diagnostics

c09b0d5

test(qualitygate): pin release target coverage

f1ce88b

test(qualitygate): fail closed on release target drift

59b6393

test(qualitygate): reject unsupported release variants

cbe0fb1

test(qualitygate): cover default release graph

7c2ca4e

fix: close layering quality gate gaps

d48c218

refactor(extension): remove internal package dependencies

8ba2431

refactor(events): move message conversion below shortcuts

d62f8dc

refactor(auth): extract authentication logging from keychain

5e23bbe

refactor(binding): split audit and config responsibilities

5157c3a

sang-neo03 requested a review from liangshuo-1 as a code owner July 25, 2026 09:59

github-actions Bot added domain/im PR touches the im domain size/XL Architecture-level or global-impact change labels Jul 25, 2026

sang-neo03 changed the base branch from refactor/package-topology to main July 25, 2026 10:03

sang-neo03 changed the title ~~refactor: clear the extension and events dependency inversions~~ refactor: enforce six-layer package boundaries and repay the recorded debt Jul 25, 2026

coderabbitai Bot reviewed Jul 25, 2026

View reviewed changes

sang-neo03 changed the title ~~refactor: enforce six-layer package boundaries and repay the recorded debt~~ refactor: enforce package boundaries and eliminate recorded dependency debt Jul 28, 2026

sang-neo03 added 4 commits July 28, 2026 14:40

refactor(config): remove unreachable legacy loaders

d7cf797

fix(lint): follow the extracted brand resolver

5bae5bb

fangshuyu-768 removed the domain/ccm PR touches the ccm domain label Jul 29, 2026

github-actions Bot added the domain/ccm PR touches the ccm domain label Jul 29, 2026

sang-neo03 added 3 commits July 29, 2026 15:27

fangshuyu-768 removed the domain/ccm PR touches the ccm domain label Jul 30, 2026

sang-neo03 added 4 commits July 30, 2026 14:26

github-actions Bot added the domain/ccm PR touches the ccm domain label Jul 30, 2026

sang-neo03 added 6 commits July 30, 2026 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: enforce package boundaries and eliminate recorded dependency debt - #2055

refactor: enforce package boundaries and eliminate recorded dependency debt#2055
sang-neo03 wants to merge 64 commits into
mainfrom
refactor/package-debt-phase2

sang-neo03 commented Jul 25, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 25, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Jul 25, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 25, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 25, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sang-neo03 commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test Plan

Related Issues

Uh oh!

coderabbitai Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 PR Preview Install Guide

🧰 CLI update

🧩 Skill update

Uh oh!

codecov Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sang-neo03 commented Jul 25, 2026 •

edited

Loading

coderabbitai Bot commented Jul 25, 2026 •

edited

Loading

github-actions Bot commented Jul 25, 2026 •

edited

Loading

codecov Bot commented Jul 25, 2026 •

edited

Loading