jared/cc-os

Watch

Fork 0

os-sdlc lint rule: query-named method reaches a write (same-file CQS) #89

New issue

Closed

opened 2026-07-22 18:29:46 +00:00 by jared · 2 comments

jared commented

2026-07-22 18:29:46 +00:00

Owner

Context

Sandi Metz review of plugins/os_sdlc/lib/os_sdlc/issue_source.rb found a query-named
public entry point that transitively performs a write, via a same-file call chain — a
narrow, mechanically-detectable form of CQS violation.

Problem

plugins/os-sdlc/lib/os_sdlc/issue_source.rb:26-44

def self.for(project, runner: Shell.new)
  tracker = project.tracker || resolve_and_persist(project, runner)
  ...
end

def self.resolve_and_persist(project, runner)
  tracker = tracker_from_config(project) || tracker_from_git_remote(project, runner)
  return nil unless tracker

  project.save_tracker(tracker)
  tracker
end

IssueSource.for reads like a pure lookup ("give me the issue source for this project")
but its call chain (for → resolve_and_persist) reaches project.save_tracker — a write —
inside the same file, one hop away, hidden behind a method name (resolve_and_persist) that
at least admits the write, but the caller-facing name (for) does not.

Detection

Inputs: the AST of one file's method definitions and their call-site bodies.
Algorithm:
1. Collect every method whose name matches a query-naming convention: for, find*,
  fetch*, get*, build* (and does NOT end in ? or !, which have their own
  conventions).
2. For each such method, build the same-file call graph reachable from its body (method
  calls to other methods defined in the same file, followed transitively, same file only
  — do not cross file/class boundaries).
3. Flag if any reachable method name matches /save|persist|write|update/i.
Failure message shown to the pipeline model (verbatim):
"IssueSource.for (query-named) calls resolve_and_persist, which calls project.save_tracker (a write) -- separate the query (locating/returning a tracker) from the command (persisting a newly-resolved tracker) into two methods, or rename the entry point so its name admits the write (e.g. resolve_and_persist_tracker) instead of reading as a pure lookup."

Correction

-      tracker = project.tracker || resolve_and_persist(project, runner)
+      tracker = project.tracker || resolve_and_persist_tracker(project, runner)
...
-    def self.resolve_and_persist(project, runner)
+    # Command: resolves AND persists -- name admits the write.
+    def self.resolve_and_persist_tracker(project, runner)
       tracker = tracker_from_config(project) || tracker_from_git_remote(project, runner)
       return nil unless tracker
       project.save_tracker(tracker)
       tracker
     end

(Minimal fix: rename so the write is visible in the name. A deeper fix would split for
into an explicit resolve query plus a caller-driven persist command, but that changes
for's public contract and is reviewer territory, not this narrow rule's scope.)

Pass/fail examples

Must fail: IssueSource.for, whose same-file call chain (for → resolve_and_persist)
reaches project.save_tracker, as in the current file.
Must pass: IssueSource.for calling only resolve_and_persist_tracker (name admits
the write) — the rule only flags query-shaped names, so renaming out of the query
convention clears it.
Must pass (scope boundary): a query method whose call chain reaches a write only
through another file/class (e.g. calling into a service object that itself writes) — out
of scope for this rule, which is deliberately limited to same-file chains; cross-file CQS
violations remain reviewer territory (false-positive risk: this rule cannot see writes
that happen outside the file being linted, and will not report them).

Provenance

Sandi Metz review of issue_source.rb, cc-os session 2026-07-22 (organic finding — not a
council/run finding).

Implementation plan

Custom RuboCop cop (batch-2 custom-cop slot; no stock cop does same-file call-graph
reachability analysis). Scope is deliberately narrow (same-file only) to keep false
positives low; implemented directly by a Fable agent, with a Codex review/audit before
merge — the audit should specifically check for false positives on query methods whose
write reachability requires this narrow scope to correctly not-fire.

## Context Sandi Metz review of `plugins/os_sdlc/lib/os_sdlc/issue_source.rb` found a query-named public entry point that transitively performs a write, via a same-file call chain — a narrow, mechanically-detectable form of CQS violation. ### Problem `plugins/os-sdlc/lib/os_sdlc/issue_source.rb:26-44` ```ruby def self.for(project, runner: Shell.new) tracker = project.tracker || resolve_and_persist(project, runner) ... end def self.resolve_and_persist(project, runner) tracker = tracker_from_config(project) || tracker_from_git_remote(project, runner) return nil unless tracker project.save_tracker(tracker) tracker end ``` `IssueSource.for` reads like a pure lookup ("give me the issue source `for` this project") but its call chain (`for` → `resolve_and_persist`) reaches `project.save_tracker` — a write — inside the same file, one hop away, hidden behind a method name (`resolve_and_persist`) that at least admits the write, but the caller-facing name (`for`) does not. ### Detection - **Inputs:** the AST of one file's method definitions and their call-site bodies. - **Algorithm:** 1. Collect every method whose name matches a query-naming convention: `for`, `find*`, `fetch*`, `get*`, `build*` (and does NOT end in `?` or `!`, which have their own conventions). 2. For each such method, build the same-file call graph reachable from its body (method calls to other methods defined in the same file, followed transitively, same file only — do not cross file/class boundaries). 3. Flag if any reachable method name matches `/save|persist|write|update/i`. - **Failure message shown to the pipeline model** (verbatim): `"IssueSource.for (query-named) calls resolve_and_persist, which calls project.save_tracker (a write) -- separate the query (locating/returning a tracker) from the command (persisting a newly-resolved tracker) into two methods, or rename the entry point so its name admits the write (e.g. resolve_and_persist_tracker) instead of reading as a pure lookup."` ### Correction ```diff - tracker = project.tracker || resolve_and_persist(project, runner) + tracker = project.tracker || resolve_and_persist_tracker(project, runner) ... - def self.resolve_and_persist(project, runner) + # Command: resolves AND persists -- name admits the write. + def self.resolve_and_persist_tracker(project, runner) tracker = tracker_from_config(project) || tracker_from_git_remote(project, runner) return nil unless tracker project.save_tracker(tracker) tracker end ``` (Minimal fix: rename so the write is visible in the name. A deeper fix would split `for` into an explicit `resolve` query plus a caller-driven `persist` command, but that changes `for`'s public contract and is reviewer territory, not this narrow rule's scope.) ### Pass/fail examples - **Must fail:** `IssueSource.for`, whose same-file call chain (`for` → `resolve_and_persist`) reaches `project.save_tracker`, as in the current file. - **Must pass:** `IssueSource.for` calling only `resolve_and_persist_tracker` (name admits the write) — the rule only flags query-shaped *names*, so renaming out of the query convention clears it. - **Must pass (scope boundary):** a query method whose call chain reaches a write *only* through another file/class (e.g. calling into a service object that itself writes) — out of scope for this rule, which is deliberately limited to same-file chains; cross-file CQS violations remain reviewer territory (false-positive risk: this rule cannot see writes that happen outside the file being linted, and will not report them). ### Provenance Sandi Metz review of `issue_source.rb`, cc-os session 2026-07-22 (organic finding — not a council/run finding). ### Implementation plan Custom RuboCop cop (batch-2 custom-cop slot; no stock cop does same-file call-graph reachability analysis). Scope is deliberately narrow (same-file only) to keep false positives low; implemented directly by a Fable agent, with a Codex review/audit before merge — the audit should specifically check for false positives on query methods whose write reachability requires this narrow scope to correctly not-fire. </content>

jared commented

2026-07-22 18:55:39 +00:00

Author

Owner

Implementation note (2026-07-22): internal inconsistency found while implementing from this ticket alone. The 'must pass' example says renaming the ENTRY POINT out of the query convention clears the offense, but the Correction diff instead renames the private helper (resolve_and_persist -> resolve_and_persist_tracker), which still matches /persist/ and would NOT clear the offense under the stated algorithm. The cop honors the algorithm + pass-example rationale (verified: renaming for clears it). Cop implemented as Sdlc/QueryMethodReachesWrite.

Implementation note (2026-07-22): internal inconsistency found while implementing from this ticket alone. The 'must pass' example says renaming the ENTRY POINT out of the query convention clears the offense, but the Correction diff instead renames the private helper (resolve_and_persist -> resolve_and_persist_tracker), which still matches /persist/ and would NOT clear the offense under the stated algorithm. The cop honors the algorithm + pass-example rationale (verified: renaming `for` clears it). Cop implemented as Sdlc/QueryMethodReachesWrite.

jared referenced this issue

2026-07-23 11:30:54 +00:00

aidd-lint rule: workflow-war-story #100

jared closed this issue

2026-07-23 19:32:49 +00:00

jared commented

2026-07-23 19:33:11 +00:00

Author

Owner

Covered: Sdlc/Structural/QueryMethodReachesWrite shipped and enabled in .rubocop.yml with tests. ADR-0061 finalizes the os-sdlc lint program; closing per commit 8e88bbe.

Covered: Sdlc/Structural/QueryMethodReachesWrite shipped and enabled in .rubocop.yml with tests. ADR-0061 finalizes the os-sdlc lint program; closing per commit 8e88bbe.

jared referenced this issue

2026-07-27 17:12:40 +00:00

workflow-war-story: #\d+ matches cross-references, not incident history (13 of 15 remaining offenses) #168

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

jared/cc-os#89

No description provided.

Rows
Columns