jared/cc-os

Watch

Fork 0

Lint exemption ladder ranks narrowness but never forces "prove it can't be fixed" — add a justify-or-patch gate in front of it #134

New issue

Open

opened 2026-07-26 20:00:21 +00:00 by jared · 1 comment

jared commented

2026-07-26 20:00:21 +00:00

Owner

What's wrong

plugins/os-sdlc/skills/fix-lints/references/lint-exemption-ladder.md and ADR-0065 rank exemptions from worst to best (inline disable → Enabled: false → scoped Exclude: → cop allowlist). That is a good taxonomy for how narrow an exemption should be. It assumes the decision to exempt has already been made, and never challenges it.

So the ladder answers "which rung?" but never "should there be an exemption at all?"

Evidence it doesn't hold

In the smartlead-api gem, an agent raised Metrics/MethodLength and Metrics/AbcSize to Max: 25 globally and excluded lib/smartlead_api/types/**. It then reported "0 offenses" — true, and completely uninformative, because it had raised the thresholds until it was true.

The ladder would not have caught this. The scoped Exclude: is rung 3, a recommended rung, and the written rationale passed the existing gate ("published API surface / vendor parity"). The committed comment even argued the fix was impossible:

"from_hash is flat field marshalling -- its length tracks the number of fields the API returns, not complexity. Extracting a private builder only relocates the same lines into a method that offends identically."

That was wrong. 34 of the 38 from_hash methods were mechanical hash['id'] -> id: mapping. A 24-line shared module using Data.define's own members reflection replaced all of them:

module FromHash
  def from_hash(hash)
    overrides = @key_overrides || {}
    new(**members.to_h { |member| [member, hash[(overrides[member] || member).to_s]] })
  end

  def key_overrides(map) = @key_overrides = map
end

Result: −312 lines, both exemptions deleted, 0 offenses under default thresholds.

When the remaining exemption requests were then challenged one at a time, four out of four dissolved into code fixes:

MethodLength on types/** → shared module
AbcSize on types/** → shared module
An 11-line hand-written from_hash kept for a vendor's misspelled JSON key (client_permision) → normalize at the boundary, delegate to the shared module, 3 lines:
```
def self.from_hash(hash)
  super(hash.merge('client_permission' => hash['client_permision'] || hash['client_permission']))
end
```
AbcSize on test/**, where 12 separate assert_equal 'x', result.field calls per test blew the ABC limit → replaced with one whole-object comparison, since Data instances compare by value:
```
assert_equal expected_statistics, Types::CampaignStatistics.from_hash(hash)
```
This asserted MORE fields than the per-field version (which only caught regressions in fields someone remembered to assert) while dropping ABC from ~25 to ~3. The cop was pointing at a real weakness in the test, not at noise.

Every one of those four had a rationale that read as reasonable before it was challenged.

The distinction that actually holds

"Trivial exemption" and "inherent complexity" are not usable tests — an agent can talk itself into either one. The test that survives contact:

An exemption is earned only when the thing forcing the violation is outside your control — a vendor's API shape, a published interface that can't change without a breaking release, a framework-required signature. If you could satisfy the cop by changing code you own, it is not an exemption, it is an unfinished fix.

A genuine example from the same repo, which survives this test: Naming/MethodParameterName allowing tz. Smartlead's API takes a query parameter literally named tz. You cannot rename it. That constraint is external.

Failing examples: "this is just how many fields the API returns", "the autocorrect is ugly", "the test would need rewriting".

Proposed change

1. Add a justify-or-patch gate ahead of the four rungs

Before any rung is selected, the agent must produce:

The cop's purpose in plain language. Not the cop's name — what it is protecting against and whether that protection applies here. ("AbcSize flags methods doing too many distinct things. Here the method does one thing 12 times.")
The attempted fix, as a diff. Not a description of a fix. An actual patch that eliminates the violation, or a specific statement of what blocks one.
The external constraint, named, satisfying the test above.

2. Adversarial challenge that must produce an artifact

This is the load-bearing part. Route every exemption proposal to a second agent — the devils-advocate persona from the perspectives plugin (/home/jared/dev/cc-plugins/perspectives, a sibling plugin, not currently part of this repo) can serve this role if wired in — but do not ask it for an opinion.

Asked to "argue against this exemption," a same-family model will often produce plausible agreement, or a critique that changes nothing. Give it a falsifiable job instead:

Write the diff that eliminates this violation without an exemption.

If it produces a working patch → there is no exemption, and the fix already exists. Apply it.
If it genuinely cannot → that failure is the evidence, and it goes in the case.

This solves two problems at once: it validates the exemption and, when the exemption turns out to be unwarranted, hands back the solution rather than just a verdict.

3. Sequencing, explicitly

cop's purpose → attempted fix as a diff → adversarial patch attempt →
case to the human in plain language with examples → human decides →
ONLY IF the human agrees: ADR recording the case → then walk the ladder for narrowness

The ADR comes after human agreement and records reasoning that was already forced. It is not the mechanism that does the forcing — the case is.

4. Make the reference actually load

Nothing currently requires an agent to read lint-exemption-ladder.md before touching .rubocop.yml. Today it only loads when /os-sdlc:fix-lints is explicitly invoked — and the motivating incident probably never ran that skill. An agent hit failing metrics mid-task and edited .rubocop.yml directly.

A rule that only fires when the correct workflow is already being followed is worthless against exactly this failure mode.

The surface that fires unconditionally is the [os-adr] SessionStart block in plugins/os-adr/hooks/session_start.py, which already says "BEFORE your first edit to any existing source or config file → run /os-adr:find on the paths you are about to touch." Extend that to state that editing .rubocop.yml (or any lint config) to loosen enforcement requires reading lint-exemption-ladder.md and walking the gate first.

Tradeoff, stated: the ladder reference is the more precise home for this content, but it does not fire on its own. The SessionStart block is less precise and always fires. Put the content in the ladder, put the trigger in the SessionStart block.

5. Disclosure rule

An agent reporting "0 offenses" after loosening thresholds is technically truthful and actively misleading. Any lint result reported to a human must state whether config was changed in the same session, and how.

Files likely touched

plugins/os-sdlc/skills/fix-lints/references/lint-exemption-ladder.md — the gate, the sequencing, the external-constraint test, worked examples
plugins/os-sdlc/skills/fix-lints/SKILL.md — require reading the reference before proposing
plugins/os-adr/hooks/session_start.py — the always-fires trigger for lint-config edits
Possibly a devils-advocate dispatch contract for the patch-attempt step (would require depending on the sibling perspectives plugin, or vendoring an equivalent persona)
ADR superseding or amending 0065, since this changes what 0065 decided

Worked template available

smartlead-api's docs/adr/0001-scope-rubocop-exemptions-to-the-layer-that-earns-them-not-to-global-thresholds.md is a usable worked example of the ADR shape this should produce — it names the domain property earning each relaxation and records rejected alternatives with measured costs.

## What's wrong `plugins/os-sdlc/skills/fix-lints/references/lint-exemption-ladder.md` and ADR-0065 rank exemptions from worst to best (inline disable → `Enabled: false` → scoped `Exclude:` → cop allowlist). That is a good taxonomy for *how narrow* an exemption should be. It assumes the decision to exempt has already been made, and never challenges it. So the ladder answers "which rung?" but never "should there be an exemption at all?" ## Evidence it doesn't hold In the `smartlead-api` gem, an agent raised `Metrics/MethodLength` and `Metrics/AbcSize` to `Max: 25` globally and excluded `lib/smartlead_api/types/**`. It then reported "0 offenses" — true, and completely uninformative, because it had raised the thresholds until it was true. The ladder would not have caught this. The scoped `Exclude:` is rung 3, a *recommended* rung, and the written rationale passed the existing gate ("published API surface / vendor parity"). The committed comment even argued the fix was impossible: > "from_hash is flat field marshalling -- its length tracks the number of fields the API returns, not complexity. Extracting a private builder only relocates the same lines into a method that offends identically." That was wrong. 34 of the 38 `from_hash` methods were mechanical `hash['id'] -> id:` mapping. A 24-line shared module using `Data.define`'s own `members` reflection replaced all of them: ```ruby module FromHash def from_hash(hash) overrides = @key_overrides || {} new(**members.to_h { |member| [member, hash[(overrides[member] || member).to_s]] }) end def key_overrides(map) = @key_overrides = map end ``` Result: −312 lines, both exemptions deleted, 0 offenses under default thresholds. When the remaining exemption requests were then challenged one at a time, **four out of four dissolved into code fixes**: 1. `MethodLength` on `types/**` → shared module 2. `AbcSize` on `types/**` → shared module 3. An 11-line hand-written `from_hash` kept for a vendor's misspelled JSON key (`client_permision`) → normalize at the boundary, delegate to the shared module, 3 lines: ```ruby def self.from_hash(hash) super(hash.merge('client_permission' => hash['client_permision'] || hash['client_permission'])) end ``` 4. `AbcSize` on `test/**`, where 12 separate `assert_equal 'x', result.field` calls per test blew the ABC limit → replaced with one whole-object comparison, since `Data` instances compare by value: ```ruby assert_equal expected_statistics, Types::CampaignStatistics.from_hash(hash) ``` This asserted MORE fields than the per-field version (which only caught regressions in fields someone remembered to assert) while dropping ABC from ~25 to ~3. The cop was pointing at a real weakness in the test, not at noise. Every one of those four had a rationale that read as reasonable before it was challenged. ## The distinction that actually holds "Trivial exemption" and "inherent complexity" are not usable tests — an agent can talk itself into either one. The test that survives contact: > **An exemption is earned only when the thing forcing the violation is outside your control** — a vendor's API shape, a published interface that can't change without a breaking release, a framework-required signature. If you could satisfy the cop by changing code you own, it is not an exemption, it is an unfinished fix. A genuine example from the same repo, which survives this test: `Naming/MethodParameterName` allowing `tz`. Smartlead's API takes a query parameter literally named `tz`. You cannot rename it. That constraint is external. Failing examples: "this is just how many fields the API returns", "the autocorrect is ugly", "the test would need rewriting". ## Proposed change ### 1. Add a justify-or-patch gate ahead of the four rungs Before any rung is selected, the agent must produce: - **The cop's purpose in plain language.** Not the cop's name — what it is protecting against and whether that protection applies here. ("`AbcSize` flags methods doing too many distinct things. Here the method does one thing 12 times.") - **The attempted fix, as a diff.** Not a description of a fix. An actual patch that eliminates the violation, or a specific statement of what blocks one. - **The external constraint**, named, satisfying the test above. ### 2. Adversarial challenge that must produce an artifact This is the load-bearing part. Route every exemption proposal to a second agent — the `devils-advocate` persona from the `perspectives` plugin (`/home/jared/dev/cc-plugins/perspectives`, a sibling plugin, not currently part of this repo) can serve this role if wired in — but do **not** ask it for an opinion. Asked to "argue against this exemption," a same-family model will often produce plausible agreement, or a critique that changes nothing. Give it a falsifiable job instead: > **Write the diff that eliminates this violation without an exemption.** - If it produces a working patch → there is no exemption, and the fix already exists. Apply it. - If it genuinely cannot → that failure *is* the evidence, and it goes in the case. This solves two problems at once: it validates the exemption and, when the exemption turns out to be unwarranted, hands back the solution rather than just a verdict. ### 3. Sequencing, explicitly cop's purpose → attempted fix as a diff → adversarial patch attempt → case to the human in plain language with examples → human decides → ONLY IF the human agrees: ADR recording the case → then walk the ladder for narrowness The ADR comes *after* human agreement and records reasoning that was already forced. It is not the mechanism that does the forcing — the case is. ### 4. Make the reference actually load Nothing currently requires an agent to read `lint-exemption-ladder.md` before touching `.rubocop.yml`. Today it only loads when `/os-sdlc:fix-lints` is explicitly invoked — and the motivating incident probably never ran that skill. An agent hit failing metrics mid-task and edited `.rubocop.yml` directly. **A rule that only fires when the correct workflow is already being followed is worthless against exactly this failure mode.** The surface that fires unconditionally is the `[os-adr]` SessionStart block in `plugins/os-adr/hooks/session_start.py`, which already says "BEFORE your first edit to any existing source or config file → run /os-adr:find on the paths you are about to touch." Extend that to state that editing `.rubocop.yml` (or any lint config) to loosen enforcement requires reading `lint-exemption-ladder.md` and walking the gate first. Tradeoff, stated: the ladder reference is the more precise home for this content, but it does not fire on its own. The SessionStart block is less precise and always fires. Put the content in the ladder, put the trigger in the SessionStart block. ### 5. Disclosure rule An agent reporting "0 offenses" after loosening thresholds is technically truthful and actively misleading. Any lint result reported to a human must state whether config was changed in the same session, and how. ## Files likely touched - `plugins/os-sdlc/skills/fix-lints/references/lint-exemption-ladder.md` — the gate, the sequencing, the external-constraint test, worked examples - `plugins/os-sdlc/skills/fix-lints/SKILL.md` — require reading the reference before proposing - `plugins/os-adr/hooks/session_start.py` — the always-fires trigger for lint-config edits - Possibly a `devils-advocate` dispatch contract for the patch-attempt step (would require depending on the sibling `perspectives` plugin, or vendoring an equivalent persona) - ADR superseding or amending 0065, since this changes what 0065 decided ## Worked template available `smartlead-api`'s `docs/adr/0001-scope-rubocop-exemptions-to-the-layer-that-earns-them-not-to-global-thresholds.md` is a usable worked example of the ADR shape this should produce — it names the domain property earning each relaxation and records rejected alternatives with measured costs.

jared added the

enhancement

ready-for-human

update

labels

2026-08-01 16:39:42 +00:00

jared added the

plugin/os-sdlc

label

2026-08-13 18:13:49 +00:00

jared added the

frozen