The comment sent readers to a follow-up in the pull request description that
does not exist there. Keep the reason in the source, where it is already
complete, and add the evidence that made the decision: applying the validator
moved four packages' expectations from /var to /private/var, because it
resolves symlinks.
97e397cf classified every startup failure, including the one the config
resolver had already classified. An unconfigured CLI comes back as
not_configured carrying "run: lark-cli config init"; wrapping it in
invalid_config put the wrong subtype in front — ProblemOf reads the outermost —
and dropped the hint entirely. A caller would be told the config is broken when
it was never written.
Pass typed errors through untouched and reserve a fresh error for the case
where the resolver gave none, where internal/unknown is the honest answer
rather than a guess at invalid_config.
Flag rejections now name the flag through WithParam, so a caller learns which
one to fix without reading the sentence, and the tests assert subtype and
param through ProblemOf instead of matching prose.
The comment claimed the binary plus two words is the deepest command path in
this CLI. It is not: generated service commands go one level further, as
`drive file.comments create_v2` in the manifest tests shows, and the cap cuts
their last word. Calling the bound a measurement invites the next reader to
raise it for a command that does not fit — which would also admit the first
positional argument, where resource identifiers live.
State it as the privacy bound it is, and add the generated-command case to the
table so the trade-off is visible next to the cases it protects.
475f04a8 stopped the command line at the first flag and dropped the
"keep three words" rule with it, on the reasoning that the command path is what
the log needs. That reasoning missed positional arguments: `api <method> <path>`
takes the path as an argument, so a document token moved from truncated to
recorded in a file that is kept for a week.
Apply both limits. Stop at the first flag, so a sensitive flag ahead of the
subcommand cannot slip through, and keep at most three words, so a positional
identifier after the command path cannot either. Removing either one fails a
test: the flag boundary alone lets the document token through, the word cap
alone lets --token=... through.
Verified case by case that nothing reaches the log that the pre-475f04a8
behaviour withheld.
LARKSUITE_CLI_LOG_DIR is validated, LARKSUITE_CLI_CONFIG_DIR is not, and the
asymmetry looks like an oversight. It is not free to remove:
validate.SafeEnvDirPath resolves symlinks, so routing CONFIG_DIR through it
changes the directory the CLI reports on any host where the path crosses one.
Applying it moved four packages' expectations from /var to /private/var on
macOS. Whether config paths should be symlink-resolved is a decision about the
on-disk contract, not a local tidy-up, so say so where the next reader looks.
Also record why the stderr capture helper uses os while the file assertions use
vfs: os.Pipe and os.Stderr are process contracts with nothing for a substituted
filesystem to intercept.
Both demo entry points reported every startup failure with fmt.Errorf, so the
self-proxy rejection was indistinguishable from a missing key file except by
reading the sentence. Classify them instead: a poisoned environment and an
unreadable config are ConfigError, flag validation is ValidationError, local
key and log file work is InternalError, and listen or serve failures are
NetworkError. Causes are attached rather than folded into the message.
The self-proxy test asserts the type with errors.As and keeps the check that
the message names the variable, so renaming the variable still fails the test
while rewording the sentence no longer does.
bf56e903 asked the next author to change both ParseBrand implementations by
cross-referencing them. That is the weakest kind of guarantee: it holds until
someone adds a brand to the package they happened to open.
Assert it instead. The constants are read out of both sources rather than
listed again, so a brand added to one package is under test immediately, and
the two parsers are compared on every declared value plus the inputs that
exercise the normalisation — case, padding, an unknown brand, a near miss.
The file sits in neither parser's package so neither owns the contract. It does
not merge the two implementations: extension/credential ships as a standalone
SDK and may not import internal, which is the constraint this branch exists to
establish.
Two behaviours carried over from internal/keychain, both left as they were when
the package moved.
FormatAuthCmdline kept the first three arguments. That protected secrets only
while no sensitive flag appeared early: a global flag in front of the
subcommand put its value straight into a file that is world-readable to the
user, kept for seven days. Today's CLI cannot reach that state — the only
persistent flag is --profile and secrets arrive through --app-secret-stdin — so
this is about the shape, not a live leak. Drop everything from the first flag
onward instead. A denylist of sensitive names would need extending whenever one
is added; the command path is what the log is for, and it lives entirely in the
leading non-flag arguments. args[0] is reduced to its base name so an absolute
install path stays out too.
logDir swallowed the error when LARKSUITE_CLI_LOG_DIR failed validation and
wrote elsewhere while the caller kept watching the path they configured. Warn
instead. This fires only on a rejected override, not on every run, and logDir
resolves once per process.
Tests cover a flag ahead of the subcommand, the absolute-path case, and that a
usable override still prints nothing.
The build-tag list was checked against a literal, so the test only caught edits
to the list itself. A tag introduced anywhere else left its files out of every
graph and the rules went quiet on them, with nothing to notice. The platform
list does not have this hole: it is cross-checked against .goreleaser.yml.
Walk the tree for //go:build constraints and require every custom tag to be
either unioned or listed as excluded. GOOS, GOARCH and toolchain terms come
from `go tool dist list` so a new port cannot look like a custom tag.
Exclusions now carry a reason that is verified rather than asserted: the two
demo tags are skipped because their files sit outside every rule's FromPrefix,
and the test fails if a file carrying one ever lands inside one. A listed
exclusion nobody uses fails too, so the list cannot rot.
Removing extension's dependency on internal left the brand rule implemented
twice, once per Brand type. The two cannot share code: extension is published
as a standalone SDK and may not import internal, which is the constraint this
branch exists to establish. Cross-reference them so a third brand is added to
both rather than to whichever one the next author happens to open.
Also correct the timestamp wrapper's comment, which has claimed "HH:mm" since
before this branch while every implementation formatted seconds too.
The package documents one logger and one file handle per process, but SetShared
overwrote the current instance on every call and the factory can be built more
than once. A second construction opened a second file, left the first one open
with no way to close it, and moved later lines to whichever workspace directory
that construction resolved.
Only the first explicit install now takes effect. A lazily created fallback is
not an explicit install, so the first real one still replaces it — and closes
the file it had opened, which needs the handle to be retained rather than
handed to log.New and forgotten. The once-guarded init becomes a mutex so the
handle can be released safely; a closed logger drops writes instead of pointing
at a file nobody reads.
Tests cover two non-nil installs and the fallback handover, and both fail if
the guard is removed.
Two packages were listed in ExceptFrom, which makes the evaluator skip the
source package before it looks at any dependency. gitcred needs keychain and
vfs; manifest-export needs the cmd root. Exempting them wholesale also cleared
every other denied import, so a later gitcred -> internal/client or
manifest-export -> events would pass the gate and never reach the registry. A
probe confirmed both slip through unreported.
Add ExceptEdges, matched on the exact (from, denied) pair, and move these two
across. ExceptFrom stays for packages whose whole job is to sit on the boundary
the rule draws: the shortcuts/common runtime gate, the cmd assembly roots, the
wrapper-main demos. Contract tests feed each package its allowed imports plus
one denied import and assert exactly one violation, so the allowed edges carry
weight instead of being asserted trivially.
Replace the uniform 0.55em half-width coefficient with per-character-type
coefficients, add font-family awareness (sans/serif), bold multiplier,
letter-spacing support, and fix padding-aware line wrapping.
- Split half-width chars into uppercase (0.57), lowercase (0.51 sans / 0.53
serif), digits (0.58), and punctuation (0.50)
- Add classify_font_family() to apply slightly wider lowercase widths for
serif fonts (Georgia, Source Han Serif/思源宋体, Times, etc.)
- Add 5% width multiplier for bold text; detect <strong>/<b>/<i>/<em> tags
and span-level bold/italic attributes in addition to content attrs
- Fix estimate_text_line_count_for_text to subtract paddingLeft/paddingRight
from available width before computing wrap lines
- Add resolve_letter_spacing and wire letterSpacing through estimate_text_width
- Extract fontFamily/bold/italic/letterSpacing into element dict during parse
chartParsedValues is a server-injected roundtrip child tag under
chartField, not an attribute. Move it from ROUNDTRIP_SXSD_ATTRS to a
new ROUNDTRIP_SXSD_TAGS set and skip the tag (and its subtree) in the
SXSD tag whitelist check.
Large low-alpha text underneath other text shapes is typically a
background design element; treat text_may_overflow_shape as info in
that case instead of warning/error.
Extract letterSpacing from content/paragraph attrs and factor it into
width and line-count estimates, and stop short-circuiting the shape
overflow check for autoFit shapes so that letterSpacing-heavy captions
under normal-auto-fit no longer escape detection.
Text-shape overflow was always reported as a warning, which let clearly
broken pages pass the lint gate. Overflow > 10px now upgrades to error;
smaller overflows stay as warning to avoid flagging near-fit cases.
Two things surfaced once CI compared this branch against main.
The panic notice in cleanupOldLogs writes to os.Stderr, which forbidigo
rejects. The line is unchanged from internal/keychain, but moving the file
makes every line new to a diff-scoped linter. This package is a leaf with no
IOStreams in scope — the same constraint defaultRuntimeDir already documents —
and a panic in background cleanup still has to be visible, so mark it the way
internal/output marks its equivalent stderr write.
Coverage of the package sat at 28.6%: the extraction brought no test for what
the logger actually writes. Add three. One pins that lines land under the
supplied RuntimeDir, which is the property that regressed when a caller
constructed the logger with empty options. One covers the nil-receiver guard
both entry points carry. One pins the seven-day retention window and the
filename patterns the prune may touch. Coverage is now 80.5%.
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.
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.
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.
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.
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.
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.
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.
--slide-id used the cobra StringArray flag type, which only accepts
repeated flags and does not split comma-separated values, unlike
--slide-number (int_array -> cobra IntSlice) which already supported
CSV input. This made the two selector flags inconsistent.
Switch --slide-id to the string_slice flag type (cobra StringSlice),
which natively supports both comma-separated and repeated values, and
update the flag readers from StrArray to StrSlice. normalizeSlideIDs
already trims/dedupes/filters blanks, and
validateSlidesScreenshotSelectorLimit already caps the combined
selector count, so both continue to apply unchanged to CSV input.
Add tests covering --slide-id CSV parsing, whitespace/duplicate
normalization, and the >10 selector limit via CSV, mirroring the
existing --slide-number coverage.
Address review feedback:
- Fix "comma-separate" -> "comma-separated" wording in the --slide-id
flag description (CodeRabbit).
- Set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() in the new screenshot
tests, per the AGENTS.md testing convention, so local configuration
state cannot leak into or be modified by the suite.
- Add a dry-run E2E test (tests/cli_e2e/slides) that pins --slide-id
CSV parsing through the built CLI binary and asserts the emitted
slide_ids request body, per the AGENTS.md dry-run E2E requirement
for shortcut flag/param changes.
- Update the lark-slides skill reference to document that --slide-id
and --slide-number both accept comma-separated values, not just
repeated flags, so agents can discover the new syntax.
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.
