ux: advisory linter for PROXIED expressions - #1235
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1235 +/- ##
==========================================
+ Coverage 98.42% 98.47% +0.04%
==========================================
Files 97 100 +3
Lines 5859 6243 +384
==========================================
+ Hits 5767 6148 +381
- Misses 79 82 +3
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
favonia
marked this pull request as draft
June 26, 2026 10:59
favonia
marked this pull request as ready for review
June 29, 2026 22:29
The PROXIED linter (R4) now flags `true && X` as a redundant term, which broke the warning-free "proxied" integration case. Drop the incidental `true &&` prefix; evaluation is identical and the case stays lint-clean.
Document why contradictory must visit every ordered pair rather than a
triangular j := i+1 loop, and add a regression test ("!sub(a.org) &&
sub(x.a.org)") that a triangular loop would miss. Migrates a verified
design insight out of branch-local planning notes into the code.
Per docs/designs/guides/testing-boundaries.markdown, rule behavior belongs in package foo_test exercised through the exported API; same-package tests are for private helpers only. R3/R4 were tested white-box via semanticFindings, which bypassed LintExpression's dedup, the shape+semantic merge, and the public message path. - Add R3/R4 cases driven through LintExpression, covering paths the internal tests missed: Boolean constants in &&/||, the contradictory negated-first regression, and the tautology that also trips R2 on the public path. - Add TestLintExpressionMerges for shape+semantic combination and dedup, a compound exclusion-only R2 branch, and a multi-domain opaque clean case. - Drop the misplaced R3/R4 behavior tests and the messages/mustParse helpers from lint_semantic_internal_test.go; keep TestLiteralRelations white-box. Keep lintExpr's key parameter explicit with a local //nolint:unparam: key is part of ParseExpression/LintExpression's keyed-message contract, not hidden coupling (docs/designs/guides/go-lint-suppressions.markdown). Claude-Session: https://claude.ai/code/session_013Dh4esJ99uUWeNhrhMZPiB
Define the four lint rules in lint.go so every R1-R4 reference in the package resolves within committed source. Claude-Session: https://claude.ai/code/session_013Dh4esJ99uUWeNhrhMZPiB
Replace the single callExpr{function, domains} AST node with two typed nodes:
isExpr holds []domain.Domain built via domain.New, and subExpr holds
[]domain.Suffix built via domain.NewSuffix. This lets the linter and evaluator
use the domain package's HasStrictSuffix methods directly instead of a local
string predicate.
Argument validation now distinguishes three outcomes:
- a valid argument is stored;
- a too-short is(...) target (domain.ErrTooFewLabels, e.g. is(org)) is
accepted and kept — it matches nothing, exactly as before — and recorded
for a later advisory;
- a sub(...) wildcard (domain.ErrWildcardSuffix, e.g. sub(*.a.org)) is
skipped and recorded, since a wildcard has no strict subdomains;
- any other malformed argument is rejected at startup.
The recorded short targets and skipped wildcards are surfaced as operator
advisories in later commits.
BREAKING CHANGE: PROXIED is(...)/sub(...) arguments that are genuinely
malformed (e.g. non-leftmost wildcards like b.*.a.org) are now rejected at
startup instead of being silently accepted and never matching. Unlike the
earlier draft of this change, sub(.) and is(.) are accepted: sub(.) matches
every domain and is(.) matches nothing.
Claude-Session: https://claude.ai/code/session_01GKwm7LFkYiACMvjeYCVMJF
When an is(...)/sub(...) argument is genuinely malformed, the parse error now names the offending domain and the underlying cause so the operator can see exactly what to fix, instead of a bare "is malformed" line. Too-short is(...) targets (domain.ErrTooFewLabels, e.g. is(org)) are accepted and kept — they match nothing, as before — but now raise a single advisory listing the joined targets, since a bare TLD or label is almost never the intended target domain name. Claude-Session: https://claude.ai/code/session_01GKwm7LFkYiACMvjeYCVMJF
A wildcard such as *.a.org has no strict subdomains, so sub(*.a.org) can never match anything. With the is/sub split, domain.NewSuffix rejects a wildcard suffix, so buildSubCall skips the argument and records it; the advisory is now emitted from reportExpressionDiagnostics instead of the LintExpression pass. This keeps the wildcard out of the AST entirely (sub(*.a.org) renders as the empty sub()), and removes the former L1 linter rule (subWildcardFinding / languageFindings) along with its now-unused imports. Claude-Session: https://claude.ai/code/session_01GKwm7LFkYiACMvjeYCVMJF
sub(.) denotes the strict subdomains of the root suffix, i.e. every domain, so it is statically constant-true. Recognize a single root suffix as an always-true atom and propagate its constancy through &&, || and negation so that sub(.), !sub(.) (constant-false) and sub(.) || is(a.org) are all flagged via the existing R3 machinery. A guard on containsAlwaysTrueAtom keeps R3 from flagging plain literal expressions like "true"/"!true", and constValue only fires when the whole expression is statically determined, so non-constant cases such as sub(.) && is(a.org) are left to the redundancy pass. Multi-arg sub() containing the root (e.g. sub(org, .)) is still opaque and is deferred to a later task. Claude-Session: https://claude.ai/code/session_01GKwm7LFkYiACMvjeYCVMJF
…(R4) A multi-arg is/sub call is semantically the disjunction of its single-atom literals. Expand such calls into positive literals so the R4 redundancy pass detects (a) redundancy within one call in any context and (b) redundancy across || terms, including atoms drawn from different calls. &&-distribution stays out of scope: a multi-arg call inside && remains opaque to conjunction analysis. Claude-Session: https://claude.ai/code/session_01GKwm7LFkYiACMvjeYCVMJF
The is/sub split added the suffix field to atomSet and the shortIsTargets / subWildcards fields to parserState. A few struct literals predating or written during the split left those fields implicit, which the exhaustruct linter flags. Enumerate every field explicitly (matching the style already used by atomsOfCall) so `golangci-lint run ./internal/domainexp/...` is clean. Claude-Session: https://claude.ai/code/session_01GKwm7LFkYiACMvjeYCVMJF
The L1 wildcard advisory built a "subdomain form" by stripping the "*." prefix. For the bare-star case sub(*) this left the string unchanged, producing nonsensical advice that suggested is(*)/sub(*) — the very forms being rejected. Emit a simpler message that just states sub(*) matches no domain, with no remediation. The normal *.X case keeps its helpful wording. Also remove unresolvable plan-internal #N tags from committed comments, rewording to refer to the behavior or the L-series name; the committed legend only defines L1 and R1-R4. Claude-Session: https://claude.ai/code/session_01NYs76uMJgSbNiT1snn2jJK
The R1-R4 lint advisories and the empty-call/short-target/sub() wildcard diagnostics built their input echo by concatenating literal double quotes around the raw expression. That left special characters unescaped: a valid expression may carry newlines, tabs, or other whitespace between tokens (the tokenizer skips any unicode space), so a manually quoted echo could spill a warning across several lines. Render the input through listSyntaxPreview (pp.QuotePreviewOrEmptyLabel at the shared AdvisoryPreviewLimit) instead, matching the comma advisories here and the parallel split in env_domain.go: advisories preview-quote the input, errors keep the full %q. The per-item values the operator must act on (the offending domain, branch, suggestion, or redundant term) are still shown in full, so capping the locator echo loses nothing actionable. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
validate_internal_test.go exercised only the exported ParseExpression contract (accepted/rejected inputs and advisory messages), matching the black-box pattern already in lint_test.go and parser_test.go. Its sole reason for living in package domainexp was the private exprString inspection hook. Per docs/designs/guides/testing-boundaries.markdown this is the export_test.go case: keep the tests in package domainexp_test and expose exprString through a minimal test-only ExprString wrapper. The wrapper is a function (not a var) to match the other export_test.go files and gochecknoglobals. The direct exprString unit tests stay internal. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
R4's redundancy passes only feed litString positive literals (both the disjunction and conjunction loops skip negated operands, since subsumption-based redundancy is sound only for positive sets), so the negated branch was unreachable through LintExpression and showed as uncovered. litString is nonetheless a total renderer of the literal type, like exprString. Add a direct white-box unit test for it next to the other private set-relation tests, covering both positive and negated is/sub. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
Three blocks read as uncovered but are reachable through ParseExpression / LintExpression, not defensive dead code; they were just missing the sub() or constant counterparts of cases the is() side already tests: - buildSubCall's flatten-error return, reached by a Boolean operator in the argument list: sub(a.org && b.org), the twin of is(true && false); - buildSubCall's malformed-suffix branch, reached by a non-wildcard invalid suffix: sub(b.*.a.org), the twin of is(b.*.a.org) in TestRejectionMessages; - hasPositiveAtom's literalExpr arm, reached when a Boolean constant sits in an || branch: (true && !is(a.org)) || is(b.org). The constant counts as a positive atom, so R2 is suppressed there; the new test pins that and the surviving R4 redundant-constant finding. buildSubCall is now 100% covered; the only branch left in hasPositiveAtom is its genuinely-unreachable default arm, the same defensive pattern the impossible-tree tests already document. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
exprStringPrec is the canonical renderer feeding user-facing warning text, so its two unreachable arms (unknown operator, unknown Expr type) now panic instead of returning "", which would have been silently corrupt output. The remaining unreachable-but-non-panicking lint arms gain comments explaining why degrading quietly (under-reporting one rule) is the safe choice there rather than crashing the daemon. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
…ad default arms The two empty default: arms in constValue and semanticFindings only existed to satisfy the exhaustive linter; they were no-ops that fell through to the same result and showed up as unreachable. Replace them with //exhaustive:ignore so the fall-through is the single, documented path. Add the missing tests for the reachable-but-untested paths these questions surfaced: constValue's sub/is/&&/|| branches (white-box), the dedup early returns in recordShortIsTarget/recordSubWildcard (black-box via repeated arguments), and invalidDomainError.Error() (interface boilerplate that production bypasses). domainexp coverage rises to 98.6%, with only the genuinely-unreachable interface markers and defensive panics left. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
Unlike a default arm this return cannot be removed: it is the compiler-required fall-through after the litIs/litSub switches, which together cover every litKind. Document that it is unreachable and that false is the safe lint answer, matching the other non-panicking unreachable arms. Claude-Session: https://claude.ai/code/session_01NdRbTg9bm2rsqn7325GMJh
favonia
changed the title
favonia
changed the title
1 task
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.
Adds an advisory linter for
PROXIEDboolean expressions: it walks the parsedExprAST and emits warnings for suspicious-but-valid expressions without rejecting them or changing evaluation, running during config load after a successful parse. Four rules across a shape pass (R1 redundant negation, R2 exclusion-only||branch) and a semantic pass overis/subset relations (R3 constant expression, R4 redundant/subsumed term); findings are a closed self-reporting set at warning severity, deduplicated by message, with R1/R4 suggesting the simpler equivalent. Multi-domain and unrecognized atoms stay opaque, so the linter never warns on a clean expression. Closes #974; theipfilterlinter, shared-package extraction, and telescoping/normalizer (R5) are deferred follow-ups.