jared/cc-os

Watch

Fork 0

Sdlc/Dry/HandRolledFetch: restrict match to side-effect-free literals and receivers #1346

New issue

Closed

opened 2026-10-25 21:19:33 +00:00 by jared · 5 comments

jared commented

2026-10-25 21:19:33 +00:00

Owner

Problem

Rewritten 2026-10-27 from docs/lint-lessons/analyses/sdlc-dry-hand-rolled-fetch.md.

Sdlc/Dry/HandRolledFetch rewrites R.key?(K) ? R[K] : D to R.fetch(K, D) when D is literal? or a constant. fetch runs D before the lookup, on every call. literal? is true for xstr, dstr, and arrays or hashes that hold calls. So h.key?(k) ? h[k] : `git rev-parse HEAD` becomes h.fetch(k, `git rev-parse HEAD`), and the command runs on every call. When R is a method call, the rewrite also changes how many times R is called, from 2 to 1. recursive_basic_literal? is true for an xstr, so that check alone does not fix the case. Also, the cop does not report h.key?(k) ? h[k] : compute, which is the same hand-rolled fetch.

Approach

Report every hand-rolled fetch ternary, whatever the default. Autocorrect only when D is a constant default and R is a simple receiver. Otherwise, do not autocorrect, and the message prescribes R.fetch(K) { D }. Fix this through node shape only: no Exclude list and no disable comment.

Required work:

Add a constant-default check to the matcher: recursive_basic_literal?, or a const node — check: present recursive_basic_literal? in plugins/os-sdlc/lib/os_sdlc/cops/dry/hand_rolled_fetch.rb
Make the constant-default check reject a default that has an xstr node at any depth — check: present xstr in plugins/os-sdlc/lib/os_sdlc/cops/dry/hand_rolled_fetch.rb
Add a simple-receiver check: lvar, ivar, cvar, gvar, const, or self.
Remove the default_literal? early return, so that a send default is also reported.
Change the message so that it names both forms: the argument form for a constant default, and the block form for every other default.
Add fixtures to plugins/os-sdlc/tests/cops/hand_rolled_fetch_test.rb for each case below.

Acceptance criteria

Given h.key?(k) ? h[k] : `git rev-parse HEAD` , when the cop runs with autocorrect, then it reports one offense and the source does not change.
Given "#{x}", [compute], { a: compute }, [`ls`], or compute as the default, when the cop runs with autocorrect, then it reports one offense and the source does not change.
Given config.key?(k) ? config[k] : "x", when the cop runs with autocorrect, then it reports one offense and the source does not change.
Given h.key?(k) ? h[k] : "default", DEFAULT, nil, or [1, :a], when the cop runs with autocorrect, then the result is h.fetch(k, <default>).
Given h.key?(k) ? other[k] : "x", when the cop runs, then it reports no offense.
Given the repo, when rubocop runs, then no Exclude list or disable comment carries this change.

Owner sketch

The change lives in RuboCop::Cop::Sdlc::Dry::HandRolledFetch, in two private predicates (constant_default?(node) and simple_receiver?(node)) and in the guard around the corrector.

Origin

Trigger: the #1325 zero-false-positive audit of the Sdlc cops
Improvised this session: none
Chain: false positive on valid code <- cop matcher too broad <- DESIGN (cop source: plugins/os-sdlc/lib/os_sdlc/cops/hand_rolled_fetch.rb)
Root candidate: none found; searched HandRolledFetch, hand_rolled_fetch, key?, fetch
Where: plugins/os-sdlc/lib/os_sdlc/cops/hand_rolled_fetch.rb
Session: eb98cdcd-6783-46a0-b7df-cf18a8fe2a56
Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os--claude-worktrees-ticket-1325/eb98cdcd-6783-46a0-b7df-cf18a8fe2a56.jsonl

VERDICT: CREATE

## Problem Rewritten 2026-10-27 from docs/lint-lessons/analyses/sdlc-dry-hand-rolled-fetch.md. `Sdlc/Dry/HandRolledFetch` rewrites `R.key?(K) ? R[K] : D` to `R.fetch(K, D)` when D is `literal?` or a constant. `fetch` runs D before the lookup, on every call. `literal?` is true for xstr, dstr, and arrays or hashes that hold calls. So ``h.key?(k) ? h[k] : `git rev-parse HEAD` `` becomes ``h.fetch(k, `git rev-parse HEAD`)``, and the command runs on every call. When R is a method call, the rewrite also changes how many times R is called, from 2 to 1. `recursive_basic_literal?` is true for an xstr, so that check alone does not fix the case. Also, the cop does not report `h.key?(k) ? h[k] : compute`, which is the same hand-rolled fetch. ## Approach Report every hand-rolled fetch ternary, whatever the default. Autocorrect only when D is a constant default and R is a simple receiver. Otherwise, do not autocorrect, and the message prescribes `R.fetch(K) { D }`. Fix this through node shape only: no `Exclude` list and no disable comment. Required work: 1. Add a constant-default check to the matcher: `recursive_basic_literal?`, or a const node — check: present `recursive_basic_literal?` in `plugins/os-sdlc/lib/os_sdlc/cops/dry/hand_rolled_fetch.rb` 2. Make the constant-default check reject a default that has an xstr node at any depth — check: present `xstr` in `plugins/os-sdlc/lib/os_sdlc/cops/dry/hand_rolled_fetch.rb` 3. Add a simple-receiver check: lvar, ivar, cvar, gvar, const, or self. 4. Remove the `default_literal?` early return, so that a send default is also reported. 5. Change the message so that it names both forms: the argument form for a constant default, and the block form for every other default. 6. Add fixtures to `plugins/os-sdlc/tests/cops/hand_rolled_fetch_test.rb` for each case below. ## Acceptance criteria 1. Given ``h.key?(k) ? h[k] : `git rev-parse HEAD` ``, when the cop runs with autocorrect, then it reports one offense and the source does not change. 2. Given `"#{x}"`, `[compute]`, `{ a: compute }`, `` [`ls`] ``, or `compute` as the default, when the cop runs with autocorrect, then it reports one offense and the source does not change. 3. Given `config.key?(k) ? config[k] : "x"`, when the cop runs with autocorrect, then it reports one offense and the source does not change. 4. Given `h.key?(k) ? h[k] : "default"`, `DEFAULT`, `nil`, or `[1, :a]`, when the cop runs with autocorrect, then the result is `h.fetch(k, <default>)`. 5. Given `h.key?(k) ? other[k] : "x"`, when the cop runs, then it reports no offense. 6. Given the repo, when rubocop runs, then no `Exclude` list or disable comment carries this change. ## Owner sketch The change lives in `RuboCop::Cop::Sdlc::Dry::HandRolledFetch`, in two private predicates (`constant_default?(node)` and `simple_receiver?(node)`) and in the guard around the corrector. ## Origin - Trigger: the #1325 zero-false-positive audit of the Sdlc cops - Improvised this session: none - Chain: false positive on valid code <- cop matcher too broad <- DESIGN (cop source: plugins/os-sdlc/lib/os_sdlc/cops/hand_rolled_fetch.rb) - Root candidate: none found; searched HandRolledFetch, hand_rolled_fetch, key?, fetch - Where: plugins/os-sdlc/lib/os_sdlc/cops/hand_rolled_fetch.rb - Session: eb98cdcd-6783-46a0-b7df-cf18a8fe2a56 - Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os--claude-worktrees-ticket-1325/eb98cdcd-6783-46a0-b7df-cf18a8fe2a56.jsonl VERDICT: CREATE

jared added the

plugin/os-sdlc

category/code-change

filed-by/agent

labels

2026-10-25 21:19:33 +00:00

jared referenced this issue

2026-10-27 16:19:15 +00:00

Lesson catalog: default lesson for every acted-on offense, not only Sdlc cops #1419

jared commented

2026-10-27 16:38:05 +00:00

Author

Owner

Skeptic DROP (depth-1, 0 hits today): proposed new cop Sdlc/Structural/EffectInLookupFallback — a shell command inside a lookup fallback (key? ternary else, fetch default/block, x[k] || cmd). Revisit when a first real site appears. Draft: docs/lint-lessons/analyses/sdlc-dry-hand-rolled-fetch.md question (b).

Skeptic DROP (depth-1, 0 hits today): proposed new cop Sdlc/Structural/EffectInLookupFallback — a shell command inside a lookup fallback (key? ternary else, fetch default/block, `x[k] || cmd`). Revisit when a first real site appears. Draft: docs/lint-lessons/analyses/sdlc-dry-hand-rolled-fetch.md question (b).

jared commented

2026-10-27 16:38:13 +00:00

Author

Owner

Body rewritten 2026-10-27 from the false-positive process; analysis and lint lesson merged to main (docs/lint-lessons/analyses/, plugins/os-sdlc/lint-lessons/).

jared referenced this issue from a commit

2026-10-27 17:10:02 +00:00

docs(os-sdlc): HandRolledFetch smell analysis and lint lesson (#1346)

jared commented

2026-10-27 21:31:26 +00:00

Author

Owner

2026-10-27: work started in fast-batch-cop-matchers (AFK). Ticket confirmed valid on main a02b9458.

2026-10-27: work started in fast-batch-cop-matchers (AFK). Ticket confirmed valid on main a02b9458.

jared commented

2026-10-27 21:48:14 +00:00

Author

Owner

Follow-up cop for a shell command in a lookup fallback (analysis section 7-8) not filed. Skeptic DROP: 0 code sites in plugins/ and ~/dev/ruby-gems; speculative; depth-1 rule. Note here if it ever shows up.

jared referenced this issue from a commit

2026-10-27 21:52:59 +00:00

fix(os-sdlc): HandRolledFetch reports every hand-rolled fetch, autocorrects only constant defaults (#1346)

jared referenced this issue from a commit

2026-10-27 21:52:59 +00:00

docs(os-sdlc): fast-batch-cop-matchers state and History row (#1346 #1358 #1371)

jared commented

2026-10-27 21:53:10 +00:00

Author

Owner

Resolution

Done: HandRolledFetch reports every key?-ternary; autocorrects to R.fetch(K, D) only for a constant default (const or recursive_basic_literal? with no xstr) on an lvar/ivar/cvar/gvar/const/self receiver; message names the argument form and the block form. Commits 7d30b7f3, 3faf91da (fast-batch-cop-matchers, merged f5310f2b, main 89ae8b3b).

Evidence: AC1-AC5 fixtures in plugins/os-sdlc/tests/cops/hand_rolled_fetch_test.rb; suite 2825 runs 0 failures; sweep over os-sdlc 0 offenses (no new hits); no Exclude or disable.

Follow-ups: Second cop (shell command in lookup fallback) dropped by skeptic: 0 sites. Lesson unchanged (already matched).

## Resolution **Done:** HandRolledFetch reports every key?-ternary; autocorrects to R.fetch(K, D) only for a constant default (const or recursive_basic_literal? with no xstr) on an lvar/ivar/cvar/gvar/const/self receiver; message names the argument form and the block form. Commits 7d30b7f3, 3faf91da (fast-batch-cop-matchers, merged f5310f2b, main 89ae8b3b). **Evidence:** AC1-AC5 fixtures in plugins/os-sdlc/tests/cops/hand_rolled_fetch_test.rb; suite 2825 runs 0 failures; sweep over os-sdlc 0 offenses (no new hits); no Exclude or disable. **Follow-ups:** Second cop (shell command in lookup fallback) dropped by skeptic: 0 sites. Lesson unchanged (already matched).

jared closed this issue

2026-10-27 21:53:10 +00:00