jared/cc-os

Watch

Fork 0

Sdlc/RequireInsideCode: rename RequireInMethod, flag any require outside the top run, guard autocorrect #1367

New issue

Closed

opened 2026-10-25 21:23:22 +00:00 by jared · 2 comments

jared commented

2026-10-25 21:23:22 +00:00

Owner

Problem

Sdlc/RequireInMethod has three defects.

Its autocorrect removes the whole source line of the require. When the require shares a line with other code, the correction deletes that code. A one-line method is deleted completely:
```
def parse(text) = (require "json"; JSON.parse(text))   # deleted by -A
def dump(obj); require "yaml"; YAML.dump(obj); end      # deleted by -A
```
It flags a require only inside a def. Its intent is "a require anywhere except the top of the file". It misses requires in blocks, conditions, begin, class bodies, and top-level requires after other code.
Its message says Move require ... for a require_relative call.

Found by the #1325 zero-false-positive audit. Redesigned with Jared on 2026-10-26 (session fe2e7204, decision B).

Approach

Rename the cop to Sdlc/RequireInsideCode and replace its matcher with the value rule in docs/plans/class-length-family/smell-require-inside-code.md (commit 0d602cf1). Fix by shape in the matcher. Never use a config Exclude list or a test-path exclusion.

Value rule: a require/require_relative call is an offense when it is not in the file's top run. The top run is the top-level statements, from the start of the file, up to the first statement that is not a require.

Carve-outs by shape (none by path):

A top-level require is not an offense when the nearest earlier top-level statement that is not a require changes load-time state: ENV[...] =/||=, $LOAD_PATH/$:, $VERBOSE =, or a call whose receiver chain starts at Warning.
A require with a non-literal argument (a glob loader such as Dir[...].each { |f| require f }, or a constant argument) is not an offense.
A require inside a top-level begin ... rescue LoadError (an optional gem) is not an offense.

Autocorrect only a stdlib require with a string-literal argument that is alone on its line (stripped line text equals the node source). Move it into the top run once. For every other shape, report the offense and do not correct.

The lint lesson exists at plugins/os-sdlc/lint-lessons/sdlc-require-inside-code.yml (commit 0d602cf1). Do not change it in this ticket.

Required work:

Rename the cop class, file, helper directory, test file, and .rubocop.yml entry from RequireInMethod to RequireInsideCode. Update every reference in code and tests, including the tests that pin the old name by path: tests/**/fast_gate_sdlc_offense_test.rb:16, commands_sdlc_lint_base_commit_test.rb:20, relay_lesson_files_test.rb:10. Leave the TSV fixtures under tests/fixtures/crc_shares/run2/ unchanged: they are historical ledger records of an earlier run.
check: absent RequireInMethod in plugins/os-sdlc/**/*.rb
Replace the matcher with the value rule and the three carve-outs above. Keep the current skip of _test.rb files, so that Sdlc/Minitest/RequireInTestMethod does not report the same node.
Guard the autocorrect: correct only when the require is alone on its line.
Build the message from the real method name.
Add must-fire fixtures: one-line endless def, def x; require "y"; ...; end, a require in a block, require "x" if cond, a class-body require, a top-level require after X = 1, a require between two top-level modules. Add must-pass fixtures: the top run, an ENV[...] ||= line before require "bundler/setup", a $LOAD_PATH.unshift line before a require, Warning.singleton_class.prepend(...) before require "rubocop", Dir[...].each { |f| require f }, a top-level begin require "x" rescue LoadError end.
Fix the 7 true hits that the probe found, so the cop passes on main:
- plugins/os-backlog/lib/backlog/shell_runner.rb:6
- plugins/os-me/hooks/cli_appender.rb:10, :16
- plugins/os-me/hooks/prediction_recorder.rb:51
- plugins/os-me/hooks/record_prediction.rb:12
- plugins/os-me/lib/os_me.rb:24
- plugins/os-sdlc/tests/all.rb:13
Route the cop in Lint::Worklist::CopPhase (plugins/os-sdlc/lib/os_sdlc/lint/worklist/cop_phase.rb) to the iterative phase (ITERATIVE_EXTRA_COPS), because most of its offenses need judgment, so that /os-sdlc:fix-lints reaches it with its lesson.
check: present Sdlc/RequireInsideCode in plugins/os-sdlc/lib/os_sdlc/lint/worklist/cop_phase.rb

Owner sketch

RuboCop::Cop::Sdlc::RequireInsideCode (the renamed cop) iterates the file's require calls, asks each whether it is an offense, reports it, and autocorrects only when the call allows it.
RequireInsideCode::TopRun (new) owns the file-level fact: which top-level statements form the top run, including the rule that a load-time-state statement (ENV, $LOAD_PATH/$:, $VERBOSE, a Warning call) does not end the run for the requires after it.
RequireInsideCode::Call (the existing RequireInMethod::Call, renamed) owns the per-call facts: the method name for the message, a literal or non-literal argument, stdlib or not, inside a top-level begin ... rescue LoadError, and alone on its source line.

Acceptance criteria

Given a one-line def whose body holds a stdlib require, when the cop autocorrects, then the method still exists and the source is unchanged.
Given a stdlib require alone on its line inside a method, when the cop autocorrects, then the call moves to the top run and the method body keeps its other lines.
Given a require_relative outside the top run, when the cop reports it, then the message names require_relative.
Given each must-fire fixture, the cop reports one offense. Given each must-pass fixture, it reports none.
Given the whole repo, when rubocop -c plugins/os-sdlc/.rubocop.yml --only Sdlc/RequireInsideCode plugins/ bin/ runs, then it reports 0 offenses. The cop stays enabled only through the os-sdlc config, as today.
Given the cop name Sdlc/RequireInsideCode, when Lint::Worklist::CopPhase computes its phase, then it returns :iterative, and Lesson::Catalog finds the lesson sdlc-require-inside-code.yml for that key.

## Problem `Sdlc/RequireInMethod` has three defects. 1. Its autocorrect removes the whole source line of the `require`. When the require shares a line with other code, the correction deletes that code. A one-line method is deleted completely: ```ruby def parse(text) = (require "json"; JSON.parse(text)) # deleted by -A def dump(obj); require "yaml"; YAML.dump(obj); end # deleted by -A ``` 2. It flags a require only inside a `def`. Its intent is "a require anywhere except the top of the file". It misses requires in blocks, conditions, `begin`, class bodies, and top-level requires after other code. 3. Its message says `Move require ...` for a `require_relative` call. Found by the #1325 zero-false-positive audit. Redesigned with Jared on 2026-10-26 (session fe2e7204, decision B). ## Approach Rename the cop to `Sdlc/RequireInsideCode` and replace its matcher with the value rule in `docs/plans/class-length-family/smell-require-inside-code.md` (commit 0d602cf1). Fix by shape in the matcher. Never use a config Exclude list or a test-path exclusion. Value rule: a `require`/`require_relative` call is an offense when it is not in the file's top run. The top run is the top-level statements, from the start of the file, up to the first statement that is not a require. Carve-outs by shape (none by path): - A top-level require is not an offense when the nearest earlier top-level statement that is not a require changes load-time state: `ENV[...] =`/`||=`, `$LOAD_PATH`/`$:`, `$VERBOSE =`, or a call whose receiver chain starts at `Warning`. - A require with a non-literal argument (a glob loader such as `Dir[...].each { |f| require f }`, or a constant argument) is not an offense. - A require inside a top-level `begin ... rescue LoadError` (an optional gem) is not an offense. Autocorrect only a stdlib `require` with a string-literal argument that is alone on its line (stripped line text equals the node source). Move it into the top run once. For every other shape, report the offense and do not correct. The lint lesson exists at `plugins/os-sdlc/lint-lessons/sdlc-require-inside-code.yml` (commit 0d602cf1). Do not change it in this ticket. Required work: 1. Rename the cop class, file, helper directory, test file, and `.rubocop.yml` entry from `RequireInMethod` to `RequireInsideCode`. Update every reference in code and tests, including the tests that pin the old name by path: `tests/**/fast_gate_sdlc_offense_test.rb:16`, `commands_sdlc_lint_base_commit_test.rb:20`, `relay_lesson_files_test.rb:10`. Leave the TSV fixtures under `tests/fixtures/crc_shares/run2/` unchanged: they are historical ledger records of an earlier run. check: absent `RequireInMethod` in `plugins/os-sdlc/**/*.rb` 2. Replace the matcher with the value rule and the three carve-outs above. Keep the current skip of `_test.rb` files, so that `Sdlc/Minitest/RequireInTestMethod` does not report the same node. 3. Guard the autocorrect: correct only when the require is alone on its line. 4. Build the message from the real method name. 5. Add must-fire fixtures: one-line endless def, `def x; require "y"; ...; end`, a require in a block, `require "x" if cond`, a class-body require, a top-level require after `X = 1`, a require between two top-level modules. Add must-pass fixtures: the top run, an `ENV[...] ||=` line before `require "bundler/setup"`, a `$LOAD_PATH.unshift` line before a require, `Warning.singleton_class.prepend(...)` before `require "rubocop"`, `Dir[...].each { |f| require f }`, a top-level `begin require "x" rescue LoadError end`. 6. Fix the 7 true hits that the probe found, so the cop passes on main: - `plugins/os-backlog/lib/backlog/shell_runner.rb:6` - `plugins/os-me/hooks/cli_appender.rb:10`, `:16` - `plugins/os-me/hooks/prediction_recorder.rb:51` - `plugins/os-me/hooks/record_prediction.rb:12` - `plugins/os-me/lib/os_me.rb:24` - `plugins/os-sdlc/tests/all.rb:13` 7. Route the cop in `Lint::Worklist::CopPhase` (`plugins/os-sdlc/lib/os_sdlc/lint/worklist/cop_phase.rb`) to the iterative phase (`ITERATIVE_EXTRA_COPS`), because most of its offenses need judgment, so that `/os-sdlc:fix-lints` reaches it with its lesson. check: present `Sdlc/RequireInsideCode` in `plugins/os-sdlc/lib/os_sdlc/lint/worklist/cop_phase.rb` ## Owner sketch - `RuboCop::Cop::Sdlc::RequireInsideCode` (the renamed cop) iterates the file's require calls, asks each whether it is an offense, reports it, and autocorrects only when the call allows it. - `RequireInsideCode::TopRun` (new) owns the file-level fact: which top-level statements form the top run, including the rule that a load-time-state statement (`ENV`, `$LOAD_PATH`/`$:`, `$VERBOSE`, a `Warning` call) does not end the run for the requires after it. - `RequireInsideCode::Call` (the existing `RequireInMethod::Call`, renamed) owns the per-call facts: the method name for the message, a literal or non-literal argument, stdlib or not, inside a top-level `begin ... rescue LoadError`, and alone on its source line. ## Acceptance criteria 1. Given a one-line `def` whose body holds a stdlib `require`, when the cop autocorrects, then the method still exists and the source is unchanged. 2. Given a stdlib `require` alone on its line inside a method, when the cop autocorrects, then the call moves to the top run and the method body keeps its other lines. 3. Given a `require_relative` outside the top run, when the cop reports it, then the message names `require_relative`. 4. Given each must-fire fixture, the cop reports one offense. Given each must-pass fixture, it reports none. 5. Given the whole repo, when `rubocop -c plugins/os-sdlc/.rubocop.yml --only Sdlc/RequireInsideCode plugins/ bin/` runs, then it reports 0 offenses. The cop stays enabled only through the os-sdlc config, as today. 6. Given the cop name `Sdlc/RequireInsideCode`, when `Lint::Worklist::CopPhase` computes its phase, then it returns `:iterative`, and `Lesson::Catalog` finds the lesson `sdlc-require-inside-code.yml` for that key.

jared added the

plugin/os-sdlc

filed-by/agent

category/code-change

labels

2026-10-25 21:23:22 +00:00

jared referenced this issue

2026-10-25 21:26:25 +00:00

Audit every Sdlc cop against the zero-false-positive standard #1325

jared referenced this issue

2026-10-26 13:57:45 +00:00

Give os-sdlc cops/ a Zeitwerk loader so tests and .rubocop.yml stop naming each cop file #1393

jared changed title from ~~Sdlc/RequireInMethod: autocorrect deletes one-line methods; skip LoadError probes; name require_relative~~ to Sdlc/RequireInsideCode: rename RequireInMethod, flag any require outside the top run, guard autocorrect

2026-10-26 14:01:44 +00:00

jared commented

2026-10-26 14:02:26 +00:00

Author

Owner

Work started on branch ticket-1367 via /os-sdlc:implement (map poodr-ticket-implementation).

jared referenced this issue from a commit

2026-10-26 14:26:32 +00:00

docs(os-sdlc): smell analysis and lint lesson for Sdlc/RequireInsideCode (#1367)

jared commented

2026-10-26 14:28:14 +00:00

Author

Owner

Resolution

Done: Renamed Sdlc/RequireInMethod to Sdlc/RequireInsideCode. It flags any require outside the file's top run, exempts non-literal arguments and top-level rescue LoadError probes, keeps requires after ENV/$LOAD_PATH/$VERBOSE/Warning setup in the top run, autocorrects only a stdlib require alone on its line, names require_relative in its message, and is routed to the iterative fix-lints phase. Lesson lint-lessons/sdlc-require-inside-code.yml and smell analysis committed. Fixed the 7 repo hits.

Evidence: Implementation 167 complete (12 dispatches, 1 reroute after a ticket-defect escalation); commits 0d602cf1, 0a934307, 5de58339 on main. Suite gate green; os-me 392 runs and os-backlog 327 runs green; rubocop --only Sdlc/RequireInsideCode over plugins/ and bin/: 2536 files, no offenses.

Follow-ups: #1393 (Zeitwerk loader for cops/, captured with skeptic verdict CREATE). Dropped: os_me.rb requiring a hooks/ file is a placement note, not a defect.

## Resolution **Done:** Renamed Sdlc/RequireInMethod to Sdlc/RequireInsideCode. It flags any require outside the file's top run, exempts non-literal arguments and top-level rescue LoadError probes, keeps requires after ENV/$LOAD_PATH/$VERBOSE/Warning setup in the top run, autocorrects only a stdlib require alone on its line, names require_relative in its message, and is routed to the iterative fix-lints phase. Lesson lint-lessons/sdlc-require-inside-code.yml and smell analysis committed. Fixed the 7 repo hits. **Evidence:** Implementation 167 complete (12 dispatches, 1 reroute after a ticket-defect escalation); commits 0d602cf1, 0a934307, 5de58339 on main. Suite gate green; os-me 392 runs and os-backlog 327 runs green; rubocop --only Sdlc/RequireInsideCode over plugins/ and bin/: 2536 files, no offenses. **Follow-ups:** #1393 (Zeitwerk loader for cops/, captured with skeptic verdict CREATE). Dropped: os_me.rb requiring a hooks/ file is a placement note, not a defect.

jared closed this issue

2026-10-26 14:28:14 +00:00

jared referenced this issue from a commit

2026-10-26 14:28:17 +00:00

feat(os-sdlc): Audited POODR boundaries for #1367; RequireInsideCode, Call, TopRun and LoadTimeState are placed reasonably and tested through the cop and CopPhase public seams. (#1367)

jared referenced this issue from a commit

2026-10-26 14:28:17 +00:00

refactor(os-sdlc): remove the old RequireInMethod cop files (#1367)

jared referenced this issue from a commit

2026-10-26 14:28:17 +00:00

docs(os-sdlc): history row for #1367

jared referenced this issue