fleet/fleetd
3
0
Fork 0
Code Issues 91 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Decide: does this project need explicit code-quality standards (clean code, design patterns, OO practice), or is the real debt somewhere else? #748

New Issue
Open
opened 2026-10-05 06:47:15 +02:00 by ltms · 4 comments
ltms commented 2026-10-05 06:47:15 +02:00
Owner
Copy Link
Copy Source

Raised by the operator on 2026-10-05:

there are many aspect of java development or general development that we have not mentioned or I did not see you apply much, some principle like: clean code, design pattern, java coding best practices (encapsulation, inheritances...) let have an architect member to evaluate the setup and answer should we have them in this project first — I didn't monitor the code quality but I think it is a mess now

Two architects are being asked to answer this independently. This body holds the evidence I measured before briefing them, so their positions can be checked against the same numbers.

What I measured, and how

All commands run on main = 7f0c4a8, 2026-10-05, from fleetd/src.

Size. find main/java -name '*.java' | wc -l → 124 files. find main/java -name '*.java' -exec cat {} + | wc -l → 36,278 lines.

Large classes. find main/java -name '*.java' -exec wc -l {} + | awk '$1>1000 && $2!="total"' | wc -l → 9 classes over 1000 lines. The largest:

Lines File
3245 config/FleetConfig.java
2738 mcp/FleetMcp.java
1906 msg/MessageService.java
1898 Fleetd.java
1862 member/HerdrPeerLauncher.java
1510 session/GitWorktrees.java
1376 session/SessionManager.java

Tests. 192 files, 62,704 lines — about 1.7 times the main source. The suite is 2118 tests, 0 failures.

Comment volume. Counting lines whose first non-space character is * or // — a heuristic, not an exact parse — grep -cE '^[[:space:]]*(\*|//)' over all main sources gives 14,369 of 36,278 lines, or 39.6%.

Comment content against this project's own rule. The same heuristic, filtered:

Pattern in a comment line Count
fleetd # 729
CB-[0-9] 671
used to / no longer / previously 148
measured 53
round [0-9] 19
verified 3

Why that last table matters more than the question as asked

CLAUDE.md already carries a mandatory rule, "Code comments describe the code as it is now". It says a code comment must never contain:

history: before this, used to, previously, no longer, now, moved to, replaces, retired, the old X, unchanged […] evidence: dates, measured, verified, tested on, host names, ports, timings […] a ticket, MR, reviewer, we or I as the reason.

So on the numbers above, roughly 1,400 comment lines break a standard this repository wrote for itself. One example, Fleetd.java:243-255, is a javadoc explaining which mutation survived a previous test round and why a later test was added — history, evidence and ticket reference in one block.

That changes the shape of the operator's question. It is not "should we adopt standards" — a standard exists and is strict. It is closer to "we have standards, they are not followed, so what is actually wrong?" The honest possibilities include: the rule is right and nobody enforces it; the rule is wrong for this codebase and should change; or the comment debt is a symptom and the real problem is that the design needs that much explaining.

What I am NOT claiming

  • I have not judged whether the code is a mess. The operator says they think it is, and says they did not monitor it. I have not read the 9 large classes with that question in mind.
  • I measured comment lines, not comment blocks, and the */// test is a heuristic. A multi-line javadoc counts once per line, so the 39.6% is not "40% of the file is documentation" in any careful sense.
  • I tried to measure parameter-list length and my command returned nothing, because declarations span lines. That zero is my instrument failing, not evidence of short parameter lists. Related open tickets suggest the opposite: #612 says Fleetd.main's injected wirings are unpinned across 16 call sites, #589 says there is 1 behavioural test across 44 wiring sites, and #587 asks for a sweep of every constructor-arg wiring site.
  • No architect position is recorded yet. Nothing below the line is decided.

Open tickets that may be symptoms of the same thing

Worth checking rather than assuming: #612, #589, #587 (wiring that no test pins), #700 (one rule, two hand-maintained lists that must agree), #578 and #586 (enum.name() on the wire at 15 sites), #572 (a lost lock release the suite would not catch), #561 (load-bearing ordering, untested).

The decision goes on this ticket

Per CLAUDE.md, architects may settle this. Both positions and the final decision are to be recorded here, including any disagreement that survives comparison.

Raised by the operator on 2026-10-05: > there are many aspect of java development or general development that we have not mentioned or I did not see you apply much, some principle like: clean code, design pattern, java coding best practices (encapsulation, inheritances...) let have an architect member to evaluate the setup and answer should we have them in this project first — I didn't monitor the code quality but I think it is a mess now Two architects are being asked to answer this independently. This body holds the evidence I measured before briefing them, so their positions can be checked against the same numbers. ## What I measured, and how All commands run on `main` = `7f0c4a8`, 2026-10-05, from `fleetd/src`. **Size.** `find main/java -name '*.java' | wc -l` → **124 files**. `find main/java -name '*.java' -exec cat {} + | wc -l` → **36,278 lines**. **Large classes.** `find main/java -name '*.java' -exec wc -l {} + | awk '$1>1000 && $2!="total"' | wc -l` → **9 classes over 1000 lines**. The largest: | Lines | File | |---|---| | 3245 | `config/FleetConfig.java` | | 2738 | `mcp/FleetMcp.java` | | 1906 | `msg/MessageService.java` | | 1898 | `Fleetd.java` | | 1862 | `member/HerdrPeerLauncher.java` | | 1510 | `session/GitWorktrees.java` | | 1376 | `session/SessionManager.java` | **Tests.** 192 files, 62,704 lines — about 1.7 times the main source. The suite is 2118 tests, 0 failures. **Comment volume.** Counting lines whose first non-space character is `*` or `//` — a heuristic, not an exact parse — `grep -cE '^[[:space:]]*(\*|//)'` over all main sources gives **14,369 of 36,278 lines, or 39.6%**. **Comment content against this project's own rule.** The same heuristic, filtered: | Pattern in a comment line | Count | |---|---| | `fleetd #` | 729 | | `CB-[0-9]` | 671 | | `used to` / `no longer` / `previously` | 148 | | `measured` | 53 | | `round [0-9]` | 19 | | `verified` | 3 | ## Why that last table matters more than the question as asked `CLAUDE.md` already carries a mandatory rule, *"Code comments describe the code as it is now"*. It says a code comment must **never** contain: > history: *before this, used to, previously, no longer, now, moved to, replaces, retired, the old X, unchanged* […] evidence: dates, *measured*, *verified*, *tested on*, host names, ports, timings […] a ticket, MR, reviewer, *we* or *I* as the reason. So on the numbers above, roughly 1,400 comment lines break a standard this repository wrote for itself. One example, `Fleetd.java:243-255`, is a javadoc explaining which mutation survived a previous test round and why a later test was added — history, evidence and ticket reference in one block. That changes the shape of the operator's question. It is not "should we adopt standards" — a standard exists and is strict. It is closer to "we have standards, they are not followed, so what is actually wrong?" The honest possibilities include: the rule is right and nobody enforces it; the rule is wrong for this codebase and should change; or the comment debt is a symptom and the real problem is that the design needs that much explaining. ## What I am NOT claiming - I have **not** judged whether the code is a mess. The operator says they think it is, and says they did not monitor it. I have not read the 9 large classes with that question in mind. - I measured **comment lines**, not comment **blocks**, and the `*`/`//` test is a heuristic. A multi-line javadoc counts once per line, so the 39.6% is not "40% of the file is documentation" in any careful sense. - I tried to measure parameter-list length and my command returned nothing, because declarations span lines. That zero is my instrument failing, not evidence of short parameter lists. Related open tickets suggest the opposite: #612 says `Fleetd.main`'s injected wirings are unpinned across 16 call sites, #589 says there is 1 behavioural test across 44 wiring sites, and #587 asks for a sweep of every constructor-arg wiring site. - No architect position is recorded yet. Nothing below the line is decided. ## Open tickets that may be symptoms of the same thing Worth checking rather than assuming: #612, #589, #587 (wiring that no test pins), #700 (one rule, two hand-maintained lists that must agree), #578 and #586 (`enum.name()` on the wire at 15 sites), #572 (a lost lock release the suite would not catch), #561 (load-bearing ordering, untested). ## The decision goes on this ticket Per `CLAUDE.md`, architects may settle this. Both positions and the final decision are to be recorded here, including any disagreement that survives comparison.
ltms commented 2026-10-05 06:49:45 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION to the brief — architects read this before you finish

Both architects were briefed with a hypothesis that is partly wrong, and this comment overrides the brief on that point. The brief is write-once; this is the newer source and it wins.

What I got wrong

My brief said the comment rule is "widely ignored", and invited you to conclude that writing more rules produces more ignored text. The second half may still be right. The first half, as stated, is not supported — I measured the stock and then talked about the flow.

The measurement I should have run first

Today's #737 work, six units merged by three different members, over the range efd9cdb~1..7f0c4a8, main sources only:

git diff efd9cdb~1..7f0c4a8 -- 'fleetd/src/main/java/*.java' \
  | grep -E '^\+' | grep -cE '^\+[[:space:]]*(\*|//)'
# 402   added comment lines

git diff efd9cdb~1..7f0c4a8 -- 'fleetd/src/main/java/*.java' \
  | grep -E '^\+' | grep -E '^\+[[:space:]]*(\*|//)' | grep -cE 'fleetd #|CB-[0-9]'
# 1     of those carry a ticket reference

The single one is * (CB-532), with no lead name.

So 1 in 402. Current practice follows the rule. The 729 fleetd # and 671 CB- comment lines in the whole tree are a stock of old debt, not evidence of what the fleet does today.

Why this matters to your answer

It separates two questions that my brief ran together:

  1. Is the rule obeyed going forward? On this one sample, yes. That weakens "rules here do not stick" as a general claim, and it weakens my suggestion that enforcement is the whole answer.
  2. Does the old debt need clearing, and is it worth it? Untouched by the measurement above. 1,400 lines of history-in-comments is still there, and a future reader cannot tell which comments are current truth and which are a 2026 lab notebook.

Those have different answers and possibly different verdicts. Please address them separately.

Treat this as evidence about my framing, not just a number

The same error is one you can make in your own answer, so it is worth naming: I measured a stock and drew a conclusion about a flow. A count over the whole tree says what accumulated; it cannot say what anyone does now. If you make a claim about practice, measure a diff over a time range. If you make a claim about debt, count the tree. Do not let one stand in for the other.

What is still open, and unchanged by this

  • I have not measured the flow over any range except today's. One sample is one sample. A wider check — say, comment lines added per month over the last six months — would say whether the rule took hold recently or was always followed and the debt predates it. I have not run that, and neither of you is required to.
  • Nothing here changes the size numbers, the test numbers, or the required reading in your brief.
  • My framing may still be wrong in the other direction too. If you think the comment debt is a side issue, say so.
## CORRECTION to the brief — architects read this before you finish Both architects were briefed with a hypothesis that is **partly wrong**, and this comment overrides the brief on that point. The brief is write-once; this is the newer source and it wins. ### What I got wrong My brief said the comment rule is "widely ignored", and invited you to conclude that writing more rules produces more ignored text. The second half may still be right. The first half, as stated, is **not supported** — I measured the stock and then talked about the flow. ### The measurement I should have run first Today's #737 work, six units merged by three different members, over the range `efd9cdb~1..7f0c4a8`, main sources only: ```bash git diff efd9cdb~1..7f0c4a8 -- 'fleetd/src/main/java/*.java' \ | grep -E '^\+' | grep -cE '^\+[[:space:]]*(\*|//)' # 402 added comment lines git diff efd9cdb~1..7f0c4a8 -- 'fleetd/src/main/java/*.java' \ | grep -E '^\+' | grep -E '^\+[[:space:]]*(\*|//)' | grep -cE 'fleetd #|CB-[0-9]' # 1 of those carry a ticket reference ``` The single one is `* (CB-532), with no lead name.` So **1 in 402**. Current practice follows the rule. The 729 `fleetd #` and 671 `CB-` comment lines in the whole tree are a **stock of old debt**, not evidence of what the fleet does today. ### Why this matters to your answer It separates two questions that my brief ran together: 1. **Is the rule obeyed going forward?** On this one sample, yes. That weakens "rules here do not stick" as a general claim, and it weakens my suggestion that enforcement is the whole answer. 2. **Does the old debt need clearing, and is it worth it?** Untouched by the measurement above. 1,400 lines of history-in-comments is still there, and a future reader cannot tell which comments are current truth and which are a 2026 lab notebook. Those have different answers and possibly different verdicts. Please address them separately. ### Treat this as evidence about my framing, not just a number The same error is one you can make in your own answer, so it is worth naming: **I measured a stock and drew a conclusion about a flow.** A count over the whole tree says what accumulated; it cannot say what anyone does now. If you make a claim about practice, measure a diff over a time range. If you make a claim about debt, count the tree. Do not let one stand in for the other. ### What is still open, and unchanged by this - I have not measured the flow over any range except today's. One sample is one sample. A wider check — say, comment lines added per month over the last six months — would say whether the rule took hold recently or was always followed and the debt predates it. I have not run that, and neither of you is required to. - Nothing here changes the size numbers, the test numbers, or the required reading in your brief. - My framing may still be wrong in the other direction too. If you think the comment debt is a side issue, say so.
ltms commented 2026-10-05 07:07:19 +02:00
Author
Owner
Copy Link
Copy Source

Architect position 1 of 2 — sol (opencode backend)

Recorded verbatim in substance. This is one position, not the decision. The second architect is still working. I have not adjudicated, and I am not merging the two into an agreement until I can compare them.

It corrected my brief, and it is right

My brief and the ticket body both say this repo's CLAUDE.md carries the comment rule. It does not. The architect checked, and I verified independently:

grep -c 'Code comments describe the code as it is now' CLAUDE.md
# 0

The rule lives in the operator's user-level CLAUDE.md, under "User-level guidance (applies to all projects)". The architect's own check found the heading at lines 77 and 104 of that file and 0 hits in the repo file.

This matters more than the violation count I opened with. The standard is not in this repository, so it is not in git, not in the build, not reviewable in a diff, and not guaranteed to reach every member — an opencode member does not read CLAUDE_CONFIG_DIR at all. Nothing in this project owns or enforces the rule. That reframes the question again: it is not "add principles" nor "enforce existing principles", it is "the principles are not part of the product".

VERDICT: mixed — "reliable but expensive to change"

Not a mess. Its evidence, from commands it ran:

  • mvn clean install → 2118 tests, 0 failures, BUILD SUCCESS.
  • JaCoCo, parsed from target/site/jacoco/jacoco.csv: line 8895/10017 = 88.80%, branch 4564/5695 = 80.14%. JaCoCo is report-only; the pom sets no coverage gate.
  • Source inventory, its own Python rglob scan: main 124 files / 36,278 lines / 39.6% comment-ish, 9 files over 1000 lines. Test 192 files / 62,705 lines / 14.6% comment-ish, 14 files over 1000 lines.
  • Per-file: Fleetd.java 904 comment lines vs 915 non-comment non-blank. MessageService.java 945 comment lines vs 868 non-comment non-blank — more comment than code.

It read Fleetd.java and MessageService.java end to end, plus FleetdAssembly.java, PackageCyclesTest.java, the pom, and selected wiring and message tests.

It flags its own policy scan as a heuristic that can double-count and false-positive, and explicitly declines to treat its any_flagged_line=1868 as an exact violation count. Good discipline; I am recording that caveat rather than the number.

DIAGNOSIS, ranked by cost

  1. Structural, highest: MessageService owns too many lifecycles. Blocking sends, async tickets, questions, answers, reply recovery, inbox draining, ticket ownership, health facts, metrics, notifications, timeouts and executor shutdown. State spread across Task, several ConcurrentHashMaps, per-session locks, volatile fields and test race hooks. Its framing: "The cost is not style. The cost is a higher chance of a race, a stranded ticket, or a reply assigned to the wrong task." It explicitly does not propose a refactor — the rule it wants is "stop adding new state ownership to this class".
  2. Structural: package and construction coupling. FleetdAssembly now does the real assembly, which it calls a good step, but forwarding AtomicReference holders are still needed to break construction-order cycles, and Fleetd remains a large home for static factories and test seams.
  3. Structural, test side: 7 test files read src/main/java as text (its scan: test_files_reading_or_walking_src_main=7). Some protect real security rules, but they depend on variable names, call spelling and method order, so a safe rename breaks a test without a behaviour change.
  4. Surface with real reading cost: comments as a lab notebook. The policy is sound, the use is not. Comments mix current invariant, defect history, and proof from past mutation runs. It stresses that MessageService has comments carrying real lock and waiter invariants that must stay — so no bulk deletion.
  5. Lowest: naming, formatting and OO style are not the problem. It found records, enums, private state, composition, small interfaces, adapters, and inheritance used where a real subtype relationship exists. Its conclusion: "The missing quality is not 'use more patterns.' It is clearer ownership, stronger package boundaries, and less historical text in source."

ANSWER: yes, adopt standards — narrow and enforceable

It rejects a broad "Clean Code"/SOLID/pattern-catalogue section as too open to interpretation, and warns it would push agents to add abstractions with no concrete need. Its proposed repo CLAUDE.md block:

## Java quality gates

- Java comments state only the current API contract or a current maintainer constraint.
  Do not put tickets, history, dates, measurements, review rationale, or test-round notes
  in Java comments.
- Keep top-level package dependencies acyclic. Do not widen a recorded cycle exception.
  Put a callback interface in the package that owns the contract.
- Test behavior or an architecture rule, not source spelling. A new test may read
  production source only when behavior and ArchUnit cannot express the rule; the test
  must state the current constraint that makes this necessary.
- Keep mutable lifecycle state under one clear owner. If a change adds independently
  changing state, extract a focused collaborator. For concurrent state, state the lock
  or atomic rule and add a deterministic interleaving test.
- Keep mutable production fields private. Do not return mutable internal collections;
  use records or immutable copies for values that cross a package boundary.

Rules it deliberately rejects, with reasons: a max class or method length, "every method must be small", "always avoid comments", any blanket inheritance rule, a required pattern list, a duplicate-code percentage, and a JaCoCo threshold. Its argument: these are weak proxies here — a class can be long because it holds a schema or many tool handlers, a short method can still split one state transition across unsafe owners, and a coverage gate rewards execution without useful assertions.

ENFORCEMENT — mechanism per rule, with gaps named

Rule Mechanism Gap it admits
Current comments only Maven-run CommentPolicyTest with a real Java comment lexer (so strings and text blocks are not scanned), plus normalized hashes as a baseline. Fails on a new banned token, and fails when a stale baseline entry could be removed. Semantic history no lexer can detect → reviewer checklist
No new or wider package cycles Replace PackageCyclesTest's broad ignoreDependency pairs with an exact baseline of allowed edges none claimed
Behaviour tests over source spelling Allow-list today's 7 source-reading tests; a new unlisted one fails the build existing debt stays until a listed file is touched
One owner per mutable lifecycle state Reviewer question + a forced-interleaving test with latch/fake clock/hook; a sleep-only race test is not accepted as proof "No general lint mechanism exists" — review is the mechanism
Encapsulation at boundaries ArchUnit rule for private non-static state; behaviour tests for returned collection snapshots "No general build mechanism exists for all aliasing cases"

It proposes splitting delivery into three units — policy text, comment ratchet, architecture gate — so policy work is never hidden inside a refactor.

COST

Its main point: the cost is not writing rules, it is making gates useful without making ordinary changes painful. A naive regex would flag strings and current contracts, hence the lexer. The edge baseline creates review friction that is only acceptable if the failure message names the exact new edge. Comment cleanup must be touched-code only, because a bulk deletion could remove a real concurrency invariant along with ticket history.

On this ticket's own concern: it says the detailed baseline format, regex list and examples belong in the test and the reviewer skill, not in CLAUDE.md, since that file is recurring context for every agent. Only the compact block above belongs there.

A verified defect came out of this → #749

It found that PackageCyclesTest's javadoc promises a new dependency between an already-excepted pair "is still checked", while the implementation passes package-wide predicates to ignoreDependency in both directions. I verified this myself and filed #749. Four of the five excepted pairs involve msg — the package this same architect ranks as most fragile.

What it did NOT check

Its own list, which I am recording rather than paraphrasing away: it did not read the other large classes end to end; did not read every test; did not read this ticket (the brief said it was optional); did not inspect the empty wiki/ submodule or run the sync check; did not inspect .mcp.json/opencode.json; did not run the contract tests, herdr, the broker or Docker; ran no IDE diagnostics; ran no mutation testing, and notes the mutation claims in source comments are historical, not checks it ran.

It also reports rg is not installed on its pane (zsh:1: command not found: rg) and that it fell back to Python.

Its quality-tool scan of the pom: maven-checkstyle-plugin=0, maven-pmd-plugin=0, spotbugs-maven-plugin=0, error_prone=0, NullAway=0, archunit-junit5=1, jacoco-maven-plugin=1.

It changed no files: git_head=7f0c4a8464c7c5b8196ce1bd61e13522a787f9ec, git_status_short=clean.

Lead's additions

  • I ran the contract profile, which this architect could not: -Pcontract clean install → MVN_EXIT=0, and 183 surefire XML files summing to 2154 tests, 0 failures, 0 errors, 0 skipped. So the Docker-backed AMQP tests pass too — 36 tests the default profile excludes.
  • I have not re-run its JaCoCo figures. Those remain its measurement, not mine.
## Architect position 1 of 2 — `sol` (opencode backend) Recorded verbatim in substance. **This is one position, not the decision.** The second architect is still working. I have not adjudicated, and I am not merging the two into an agreement until I can compare them. ### It corrected my brief, and it is right My brief and the ticket body both say this repo's `CLAUDE.md` carries the comment rule. **It does not.** The architect checked, and I verified independently: ```bash grep -c 'Code comments describe the code as it is now' CLAUDE.md # 0 ``` The rule lives in the operator's **user-level** `CLAUDE.md`, under "User-level guidance (applies to all projects)". The architect's own check found the heading at lines 77 and 104 of that file and 0 hits in the repo file. This matters more than the violation count I opened with. The standard is **not in this repository**, so it is not in git, not in the build, not reviewable in a diff, and not guaranteed to reach every member — an opencode member does not read `CLAUDE_CONFIG_DIR` at all. Nothing in this project owns or enforces the rule. That reframes the question again: it is not "add principles" nor "enforce existing principles", it is "the principles are not part of the product". ### VERDICT: mixed — "reliable but expensive to change" Not a mess. Its evidence, from commands it ran: - `mvn clean install` → 2118 tests, 0 failures, BUILD SUCCESS. - JaCoCo, parsed from `target/site/jacoco/jacoco.csv`: line 8895/10017 = **88.80%**, branch 4564/5695 = **80.14%**. JaCoCo is report-only; the pom sets no coverage gate. - Source inventory, its own Python `rglob` scan: main 124 files / 36,278 lines / 39.6% comment-ish, 9 files over 1000 lines. Test 192 files / 62,705 lines / 14.6% comment-ish, 14 files over 1000 lines. - Per-file: `Fleetd.java` 904 comment lines vs 915 non-comment non-blank. `MessageService.java` **945 comment lines vs 868 non-comment non-blank** — more comment than code. It read `Fleetd.java` and `MessageService.java` end to end, plus `FleetdAssembly.java`, `PackageCyclesTest.java`, the pom, and selected wiring and message tests. It flags its own policy scan as a heuristic that can double-count and false-positive, and explicitly declines to treat its `any_flagged_line=1868` as an exact violation count. Good discipline; I am recording that caveat rather than the number. ### DIAGNOSIS, ranked by cost 1. **Structural, highest: `MessageService` owns too many lifecycles.** Blocking sends, async tickets, questions, answers, reply recovery, inbox draining, ticket ownership, health facts, metrics, notifications, timeouts and executor shutdown. State spread across `Task`, several `ConcurrentHashMap`s, per-session locks, volatile fields and test race hooks. Its framing: *"The cost is not style. The cost is a higher chance of a race, a stranded ticket, or a reply assigned to the wrong task."* It explicitly does **not** propose a refactor — the rule it wants is "stop adding new state ownership to this class". 2. **Structural: package and construction coupling.** `FleetdAssembly` now does the real assembly, which it calls a good step, but forwarding `AtomicReference` holders are still needed to break construction-order cycles, and `Fleetd` remains a large home for static factories and test seams. 3. **Structural, test side: 7 test files read `src/main/java` as text** (its scan: `test_files_reading_or_walking_src_main=7`). Some protect real security rules, but they depend on variable names, call spelling and method order, so a safe rename breaks a test without a behaviour change. 4. **Surface with real reading cost: comments as a lab notebook.** The policy is sound, the use is not. Comments mix current invariant, defect history, and proof from past mutation runs. It stresses that `MessageService` has comments carrying **real lock and waiter invariants that must stay** — so no bulk deletion. 5. **Lowest: naming, formatting and OO style are not the problem.** It found records, enums, private state, composition, small interfaces, adapters, and inheritance used where a real subtype relationship exists. Its conclusion: *"The missing quality is not 'use more patterns.' It is clearer ownership, stronger package boundaries, and less historical text in source."* ### ANSWER: yes, adopt standards — narrow and enforceable It rejects a broad "Clean Code"/SOLID/pattern-catalogue section as too open to interpretation, and warns it would push agents to add abstractions with no concrete need. Its proposed repo `CLAUDE.md` block: ``` ## Java quality gates - Java comments state only the current API contract or a current maintainer constraint. Do not put tickets, history, dates, measurements, review rationale, or test-round notes in Java comments. - Keep top-level package dependencies acyclic. Do not widen a recorded cycle exception. Put a callback interface in the package that owns the contract. - Test behavior or an architecture rule, not source spelling. A new test may read production source only when behavior and ArchUnit cannot express the rule; the test must state the current constraint that makes this necessary. - Keep mutable lifecycle state under one clear owner. If a change adds independently changing state, extract a focused collaborator. For concurrent state, state the lock or atomic rule and add a deterministic interleaving test. - Keep mutable production fields private. Do not return mutable internal collections; use records or immutable copies for values that cross a package boundary. ``` Rules it deliberately **rejects**, with reasons: a max class or method length, "every method must be small", "always avoid comments", any blanket inheritance rule, a required pattern list, a duplicate-code percentage, and a JaCoCo threshold. Its argument: these are weak proxies here — a class can be long because it holds a schema or many tool handlers, a short method can still split one state transition across unsafe owners, and a coverage gate rewards execution without useful assertions. ### ENFORCEMENT — mechanism per rule, with gaps named | Rule | Mechanism | Gap it admits | |---|---|---| | Current comments only | Maven-run `CommentPolicyTest` with a real Java comment **lexer** (so strings and text blocks are not scanned), plus normalized hashes as a baseline. Fails on a new banned token, and fails when a stale baseline entry could be removed. | Semantic history no lexer can detect → reviewer checklist | | No new or wider package cycles | Replace `PackageCyclesTest`'s broad `ignoreDependency` pairs with an **exact baseline of allowed edges** | none claimed | | Behaviour tests over source spelling | Allow-list today's 7 source-reading tests; a new unlisted one fails the build | existing debt stays until a listed file is touched | | One owner per mutable lifecycle state | Reviewer question + a forced-interleaving test with latch/fake clock/hook; a sleep-only race test is not accepted as proof | **"No general lint mechanism exists"** — review is the mechanism | | Encapsulation at boundaries | ArchUnit rule for private non-static state; behaviour tests for returned collection snapshots | **"No general build mechanism exists for all aliasing cases"** | It proposes splitting delivery into three units — policy text, comment ratchet, architecture gate — so policy work is never hidden inside a refactor. ### COST Its main point: the cost is not writing rules, it is making gates useful without making ordinary changes painful. A naive regex would flag strings and current contracts, hence the lexer. The edge baseline creates review friction that is only acceptable if the failure message names the exact new edge. Comment cleanup must be touched-code only, because a bulk deletion could remove a real concurrency invariant along with ticket history. On this ticket's own concern: it says the detailed baseline format, regex list and examples belong in the test and the reviewer skill, **not** in `CLAUDE.md`, since that file is recurring context for every agent. Only the compact block above belongs there. ### A verified defect came out of this → #749 It found that `PackageCyclesTest`'s javadoc promises a new dependency between an already-excepted pair "is still checked", while the implementation passes package-wide predicates to `ignoreDependency` in both directions. I verified this myself and filed **#749**. Four of the five excepted pairs involve `msg` — the package this same architect ranks as most fragile. ### What it did NOT check Its own list, which I am recording rather than paraphrasing away: it did not read the other large classes end to end; did not read every test; **did not read this ticket** (the brief said it was optional); did not inspect the empty `wiki/` submodule or run the sync check; did not inspect `.mcp.json`/`opencode.json`; **did not run the contract tests, herdr, the broker or Docker**; ran no IDE diagnostics; ran no mutation testing, and notes the mutation claims in source comments are historical, not checks it ran. It also reports `rg` is not installed on its pane (`zsh:1: command not found: rg`) and that it fell back to Python. Its quality-tool scan of the pom: `maven-checkstyle-plugin=0, maven-pmd-plugin=0, spotbugs-maven-plugin=0, error_prone=0, NullAway=0, archunit-junit5=1, jacoco-maven-plugin=1`. It changed no files: `git_head=7f0c4a8464c7c5b8196ce1bd61e13522a787f9ec`, `git_status_short=clean`. ### Lead's additions - I ran the **contract** profile, which this architect could not: `-Pcontract clean install` → `MVN_EXIT=0`, and 183 surefire XML files summing to **2154 tests, 0 failures, 0 errors, 0 skipped**. So the Docker-backed AMQP tests pass too — 36 tests the default profile excludes. - I have **not** re-run its JaCoCo figures. Those remain its measurement, not mine.
ltms commented 2026-10-05 07:16:19 +02:00
Author
Owner
Copy Link
Copy Source

Lead: coverage verified, and the two figures differ for a good reason

Architect 1's JaCoCo numbers were its measurement, not mine. I have now run it myself and the two disagree — both correctly.

I parsed fleetd/target/site/jacoco/jacoco.csv after my own -Pcontract clean install:

Suite Line Branch
Default profile — architect's run, 2118 tests 8895/10017 = 88.80% 4564/5695 = 80.14%
-Pcontract — my run, 2154 tests 9114/10017 = 90.99% 4620/5695 = 81.12%

The denominators are identical, so this is not a different measurement of the same thing. The 36 @Tag("contract") tests cover 219 lines and 56 branches that nothing in the hermetic suite reaches.

That is worth stating plainly because it is an easy wrong conclusion: "88.80%" is not this project's coverage, it is the coverage of the suite that runs without Docker. Quoting either number without naming its profile is misleading.

The gap I went looking for is not there

If CI ran only the default profile, those 219 lines would be covered on no machine but a developer's. I checked .gitea/workflows/ci.yml:

  • the build job runs mvn -B clean install, which inherits excludedGroups=contract — deliberately Docker-free;
  • a separate contract job runs mvn -B -Pcontract test -Dgroups=contract against a real broker supplied as a service (line 122).

So both halves run in CI. The split is intentional and documented in the workflow's own comments. The file also records that an earlier form of this silently excluded every other contract test from CI, so the current shape is a fix, not an accident.

No finding. I am recording the negative result because "I checked whether CI covers the Docker-only paths" is worth exactly as much as finding a hole, and the next person should not have to repeat it.

Why this bears on the decision

It is evidence against the framing I opened this ticket with. The enforcement story here is better than "a standard nobody enforces" suggested: there is an ArchUnit gate, a two-job CI split with a real broker, and 2154 tests at ~91% line coverage. What is missing is narrower than it first looked — no comment policy check, no lint or static-analysis plugin (checkstyle=0, pmd=0, spotbugs=0, error_prone=0, NullAway=0 per architect 1's pom scan), and no coverage gate, which architect 1 argues against adding anyway.

Whoever adjudicates should weigh that: the question is which small number of gates are missing, not whether this project has any.

## Lead: coverage verified, and the two figures differ for a good reason Architect 1's JaCoCo numbers were its measurement, not mine. I have now run it myself and the two disagree — both correctly. I parsed `fleetd/target/site/jacoco/jacoco.csv` after my own `-Pcontract clean install`: | Suite | Line | Branch | |---|---|---| | Default profile — architect's run, 2118 tests | 8895/10017 = **88.80%** | 4564/5695 = **80.14%** | | `-Pcontract` — my run, 2154 tests | 9114/10017 = **90.99%** | 4620/5695 = **81.12%** | The denominators are identical, so this is not a different measurement of the same thing. The 36 `@Tag("contract")` tests cover **219 lines and 56 branches that nothing in the hermetic suite reaches**. That is worth stating plainly because it is an easy wrong conclusion: "88.80%" is not this project's coverage, it is the coverage of the suite that runs without Docker. Quoting either number without naming its profile is misleading. ## The gap I went looking for is not there If CI ran only the default profile, those 219 lines would be covered on no machine but a developer's. I checked `.gitea/workflows/ci.yml`: - the `build` job runs `mvn -B clean install`, which inherits `excludedGroups=contract` — deliberately Docker-free; - a separate **`contract` job** runs `mvn -B -Pcontract test -Dgroups=contract` against a real broker supplied as a service (line 122). So both halves run in CI. The split is intentional and documented in the workflow's own comments. The file also records that an earlier form of this silently excluded every other contract test from CI, so the current shape is a fix, not an accident. **No finding.** I am recording the negative result because "I checked whether CI covers the Docker-only paths" is worth exactly as much as finding a hole, and the next person should not have to repeat it. ## Why this bears on the decision It is evidence against the framing I opened this ticket with. The enforcement story here is better than "a standard nobody enforces" suggested: there is an ArchUnit gate, a two-job CI split with a real broker, and 2154 tests at ~91% line coverage. What is missing is narrower than it first looked — no comment policy check, no lint or static-analysis plugin (`checkstyle=0, pmd=0, spotbugs=0, error_prone=0, NullAway=0` per architect 1's pom scan), and no coverage gate, which architect 1 argues against adding anyway. Whoever adjudicates should weigh that: the question is which **small number** of gates are missing, not whether this project has any.
ltms commented 2026-10-05 07:32:55 +02:00
Author
Owner
Copy Link
Copy Source

Architect position 2 of 2 — opus architect — and the DECISION

Position 2 arrived, but not through the bridge. Its ticket died on the #588 timeout at exactly 1800s while fleet_status still said working. I recovered the full reply from the member's own transcript, where a fleet_reply call records its content whether or not delivery works. Nothing was lost, but that is a rescue, not a route — see my comment on #588.

Position 2, in short

Verdict: MIXED. Same as position 1, reached independently.

Its central move is to attack my size metric. It counted code lines, not total lines:

file total comment code
mcp/FleetMcp.java 2738 1058 1544
config/FleetConfig.java 3245 1582 1504
member/HerdrPeerLauncher.java 1862 891 857
Fleetd.java 1898 972 847
msg/MessageService.java 1906 1056 757

I re-ran every number in that table myself and it is exact. Whole main source: 124 files, 36,278 lines, of which 17,850 are code. Files over 1000 total lines: 9. Files over 1000 code lines: 2.

So my "9 classes over 1000 lines" was mostly a comment-volume artifact. MessageService is a 757-line class wearing 1056 lines of comment.

Its other measurements, all of which I re-ran and confirmed:

  • FleetdAssembly.assembleAndStart spans lines 124..575 — 452 lines, 305 of them code. The longest method in the codebase.
  • 54 static factories on Fleetd (^ static ), a class whose only production entry is main.
  • 74 test classes in the root package; 20 *WiringTest, 10 *AssemblyTest, 7 *LookupTest.
  • 8 test files read src/main/java as text. (Position 1 said 7. Position 2 is right; I listed all 8.)
  • 0 public non-final fields. 12 class X extends Y, of which 8 are exception subclasses. 120 records.
  • 44 distinct test-class names referenced from main source in 76 places, of which 2 are dead: FleetdCompletionResolverWiringTest (Fleetd.java:738, mcp/FleetMcp.java:354) and FleetdLeadRolloverWiringTest (mcp/FleetMcp.java:354). Both were renamed to *AssemblyTest.
  • 5 orphaned javadoc blocks — a javadoc followed by another javadoc, so javac attaches only the second. Fleetd.java:339 is 16 lines describing exhaustedPatternCoverageLine; that method sits at line 389 with no javadoc of its own.

It also corrected itself twice unprompted, including one correction in my favour: all four claude-code profiles do receive the operator's comment rule, because ltms and gx10 are both symlinks to the same file. My "not delivered" was wrong for claude-code members. "Not part of the project" is right for everyone.

Where the two architects AGREE — independently, on separate backends

This is the larger part of the answer, and it settles the operator's question.

  1. Not a mess. Both say MIXED. Both measured clean encapsulation (0 public mutable fields), sparing and correct inheritance, heavy use of records, and correct existing patterns.
  2. No Clean Code manifesto, no SOLID section, no design-pattern catalogue. Both reject it in writing. Position 1: it would "push agents to add abstractions with no concrete need." Position 2: the patterns are already used correctly, so naming them "would add text and change no behaviour."
  3. No class or method length limit. No coverage gate. Both reject these as weak proxies.
  4. The comment rule is not in this repo, and that is the real defect in the standard. Both found this independently. It lives in the operator's personal global config, so it is not in git, never reviewed, and not versioned with the code.
  5. Comment debt is real but it is NOT the top cost. Both rank it below the structural problems. Both warn against bulk deletion because some comments carry live lock and waiter invariants.
  6. The source-text tests are a liability — they depend on variable names and call spelling, so a safe rename breaks a test with no behaviour change.
  7. Both named their own unenforceable rules instead of hiding them in a table. Position 1: "No general lint mechanism exists" for state ownership. Position 2: rule 1d "has no mechanism and is the most important rule I propose." I am keeping both admissions.

Where they DISAGREE — and why it is smaller than it looks

Each ranked a different class as the #1 structural problem:

  • Position 1: MessageService owns too many lifecycles. Blocking sends, async tickets, questions, answers, reply recovery, inbox draining, ownership, health, metrics, timeouts, executor shutdown. "The cost is not style. The cost is a higher chance of a race, a stranded ticket, or a reply assigned to the wrong task."
  • Position 2: the composition root cannot be tested, and the code shape is paying for it. 452-line assembleAndStart, 54 static factories, half the root-package tests pinning wiring rather than behaviour, 8 tests asserting on source text. "The team has been treating a design problem as a testing problem."

This is not a conflict of judgement. It is an artifact of what each one read. Position 2 states plainly that it never opened MessageService. Position 1 read both MessageService and FleetdAssembly. So only position 1 read both candidates — and it still ranked MessageService first. Position 2's ranking cannot outweigh that, because it never looked at the alternative.

Both are real, and both appear in the other's list at #2.

What neither of them named, and I found while checking their rankings

The two #1 candidates are the same disease, and there is a third symptom that proves it.

grep -rcE '([Ff]orTest|RaceHook)' --include='*.java' src/main/java
  msg/MessageService.java : 49
  msg/ReplyPushLoop.java  :  1

17 distinct test-only seam names live in production source, 49 mentions, and all but one are in MessageService. Seven are volatile Runnable race hooks: finishAsyncTaskRaceHook, afterFinishAsyncTaskCompleteHookForTest, timeoutCancellationRaceHookForTest, replyOrphanTurnIdRaceHookForTest, abandonCleanupHookForTest, askTimeoutRaceHookForTest, answerAskLapseRaceHookForTest.

So the single cause behind both rankings is this: when something here is hard to test, we change the shape of production code to let a test reach it. On Fleetd that produced 54 static factories. On MessageService it produced 7 mutable race hooks and 6 concurrent maps. On 8 test files it produced assertions on source text. Three different symptoms, one habit.

That is the finding the operator is reacting to, and neither "clean code" nor "design patterns" names it.

DECISION

Answer to the operator's question: yes, adopt standards — but not the ones the question implies. No Clean Code section, no SOLID, no pattern catalogue. Both architects reject those, and the measurements support them: encapsulation and inheritance are already fine here.

Adopt five rules. Each one exists because it has already cost us something measured.

# Rule From Mechanism Enforced?
1 Comments state only the current contract or a current maintainer constraint. No tickets, history, dates, measurements or review rationale. both CommentPolicyTest with a real Java lexer + normalized-hash baseline yes
2 A javadoc block stops at 30 lines. Over that, the knowledge moves to docs/<subject>.md and is linked in one line. pos. 2 same test; freeze today's 64 over-long blocks, ratchet down yes
3 No new mutable lifecycle state owners, and no new test seam in production source. pos. 1 + my finding cap ([Ff]orTest|RaceHook) at today's 49; review for the rest partly
4 The composition root does not grow, and a new static factory is not how you test wiring. pos. 2 cap assembleAndStart at 452 lines and ^ static on Fleetd at 54; ratchet both down partly
5 No new package cycle, and no widening of a recorded one. No new source-text test. both exact edge baseline replacing the broad ignoreDependency pairs; allow-list today's 8 source-reading tests yes

Rulings on the three places the architects conflicted

Javadoc length limit — ADOPTED, over position 1's objection. Position 1 rejects length limits. Its stated reason does not transfer: it argued a class can be long because it holds a schema or many tool handlers. There is no equivalent honest reason for a 235-line javadoc block (config/ConfigRef.java:17). And position 2's rule says move the content, not delete it, which answers position 1's real worry about losing concurrency invariants.

Mechanism — position 1's lexer wins over position 2's regex. Both architects used regex classifiers and both reported false positives in their own output. A regex over Java source will flag string literals and text blocks. The gate must lex.

Scope of cleanup — position 1's "touched-code only" wins. No bulk comment sweep. MessageService comments carry live lock and waiter invariants, and a sweep is how we would lose one.

What is NOT enforced, stated plainly

Rules 3 and 4 have caps, and a cap is not the rule. A number can stop the count growing; it cannot tell a good decomposition from a bad one. Position 2 said this about its own most important rule and refused to pretend otherwise. I am keeping that refusal in the decision: the judgement half of rules 3 and 4 is a review obligation, not a build check.

Cost, not hidden

The repo CLAUDE.md is 504 lines / 6,335 words / 40,219 bytes today, and every session loads it. Position 2 costed its own proposal at ~34 lines, about +7%, and declined to call that free. I am capping the new section at ~20 lines, because position 1 is right that the baseline format, the token list and the examples belong in the test and the reviewer skill, not in recurring context.

Delivery order — #749 first

  1. #749 — the verified PackageCyclesTest javadoc-vs-code defect, found by position 1. It must be fixed before rule 5 lands, because rule 5 replaces the mechanism it documents.
  2. The 2 dead test names and 5 orphaned javadocs. Comment-only edits, no behaviour risk. These are fixed, not frozen — they are already wrong today.
  3. The policy text into the repo CLAUDE.md, with the "goes in the ADR" line corrected to docs/<subject>.md. This project has no ADR directory, which is why that line was unfollowable: it sent knowledge to a destination that does not exist.
  4. The gates, as three separate units per position 1's split, so policy work is never hidden inside a refactor.

Both architect panes are released after this comment.

## Architect position 2 of 2 — `opus` architect — and the DECISION Position 2 arrived, but **not through the bridge**. Its ticket died on the #588 timeout at exactly 1800s while `fleet_status` still said `working`. I recovered the full reply from the member's own transcript, where a `fleet_reply` call records its `content` whether or not delivery works. Nothing was lost, but that is a rescue, not a route — see my comment on #588. ### Position 2, in short **Verdict: MIXED.** Same as position 1, reached independently. Its central move is to attack my size metric. It counted code lines, not total lines: | file | total | comment | **code** | |---|---|---|---| | mcp/FleetMcp.java | 2738 | 1058 | **1544** | | config/FleetConfig.java | 3245 | 1582 | **1504** | | member/HerdrPeerLauncher.java | 1862 | 891 | **857** | | Fleetd.java | 1898 | 972 | **847** | | msg/MessageService.java | 1906 | 1056 | **757** | **I re-ran every number in that table myself and it is exact.** Whole main source: 124 files, 36,278 lines, of which **17,850 are code**. Files over 1000 *total* lines: 9. Files over 1000 *code* lines: **2**. So my "9 classes over 1000 lines" was mostly a comment-volume artifact. `MessageService` is a 757-line class wearing 1056 lines of comment. Its other measurements, all of which I re-ran and confirmed: - `FleetdAssembly.assembleAndStart` spans lines **124..575 — 452 lines, 305 of them code**. The longest method in the codebase. - **54** static factories on `Fleetd` (`^ static `), a class whose only production entry is `main`. - **74** test classes in the root package; **20** `*WiringTest`, **10** `*AssemblyTest`, **7** `*LookupTest`. - **8** test files read `src/main/java` as text. (Position 1 said 7. Position 2 is right; I listed all 8.) - **0** public non-final fields. **12** `class X extends Y`, of which 8 are exception subclasses. **120** records. - **44** distinct test-class names referenced from main source in **76** places, of which **2 are dead**: `FleetdCompletionResolverWiringTest` (`Fleetd.java:738`, `mcp/FleetMcp.java:354`) and `FleetdLeadRolloverWiringTest` (`mcp/FleetMcp.java:354`). Both were renamed to `*AssemblyTest`. - **5** orphaned javadoc blocks — a javadoc followed by another javadoc, so `javac` attaches only the second. `Fleetd.java:339` is 16 lines describing `exhaustedPatternCoverageLine`; that method sits at line 389 **with no javadoc of its own**. It also corrected itself twice unprompted, including one correction in my favour: all four `claude-code` profiles *do* receive the operator's comment rule, because `ltms` and `gx10` are both symlinks to the same file. My "not delivered" was wrong for claude-code members. "Not part of the project" is right for everyone. --- ## Where the two architects AGREE — independently, on separate backends This is the larger part of the answer, and it settles the operator's question. 1. **Not a mess.** Both say MIXED. Both measured clean encapsulation (0 public mutable fields), sparing and correct inheritance, heavy use of records, and correct existing patterns. 2. **No Clean Code manifesto, no SOLID section, no design-pattern catalogue.** Both reject it in writing. Position 1: it would "push agents to add abstractions with no concrete need." Position 2: the patterns are already used correctly, so naming them "would add text and change no behaviour." 3. **No class or method length limit. No coverage gate.** Both reject these as weak proxies. 4. **The comment rule is not in this repo, and that is the real defect in the standard.** Both found this independently. It lives in the operator's personal global config, so it is not in git, never reviewed, and not versioned with the code. 5. **Comment debt is real but it is NOT the top cost.** Both rank it below the structural problems. Both warn against bulk deletion because some comments carry live lock and waiter invariants. 6. **The source-text tests are a liability** — they depend on variable names and call spelling, so a safe rename breaks a test with no behaviour change. 7. **Both named their own unenforceable rules** instead of hiding them in a table. Position 1: *"No general lint mechanism exists"* for state ownership. Position 2: rule 1d *"has no mechanism and is the most important rule I propose."* I am keeping both admissions. ## Where they DISAGREE — and why it is smaller than it looks Each ranked a different class as the #1 structural problem: - **Position 1: `MessageService` owns too many lifecycles.** Blocking sends, async tickets, questions, answers, reply recovery, inbox draining, ownership, health, metrics, timeouts, executor shutdown. *"The cost is not style. The cost is a higher chance of a race, a stranded ticket, or a reply assigned to the wrong task."* - **Position 2: the composition root cannot be tested, and the code shape is paying for it.** 452-line `assembleAndStart`, 54 static factories, half the root-package tests pinning wiring rather than behaviour, 8 tests asserting on source text. *"The team has been treating a design problem as a testing problem."* **This is not a conflict of judgement. It is an artifact of what each one read.** Position 2 states plainly that it never opened `MessageService`. Position 1 read both `MessageService` and `FleetdAssembly`. So only position 1 read both candidates — and it still ranked `MessageService` first. Position 2's ranking cannot outweigh that, because it never looked at the alternative. Both are real, and both appear in the other's list at #2. ## What neither of them named, and I found while checking their rankings The two #1 candidates are the same disease, and there is a third symptom that proves it. ``` grep -rcE '([Ff]orTest|RaceHook)' --include='*.java' src/main/java msg/MessageService.java : 49 msg/ReplyPushLoop.java : 1 ``` **17 distinct test-only seam names live in production source, 49 mentions, and all but one are in `MessageService`.** Seven are `volatile Runnable` race hooks: `finishAsyncTaskRaceHook`, `afterFinishAsyncTaskCompleteHookForTest`, `timeoutCancellationRaceHookForTest`, `replyOrphanTurnIdRaceHookForTest`, `abandonCleanupHookForTest`, `askTimeoutRaceHookForTest`, `answerAskLapseRaceHookForTest`. So the single cause behind both rankings is this: **when something here is hard to test, we change the shape of production code to let a test reach it.** On `Fleetd` that produced 54 static factories. On `MessageService` it produced 7 mutable race hooks and 6 concurrent maps. On 8 test files it produced assertions on source text. Three different symptoms, one habit. That is the finding the operator is reacting to, and neither "clean code" nor "design patterns" names it. --- # DECISION **Answer to the operator's question: yes, adopt standards — but not the ones the question implies.** No Clean Code section, no SOLID, no pattern catalogue. Both architects reject those, and the measurements support them: encapsulation and inheritance are already fine here. Adopt **five rules**. Each one exists because it has already cost us something measured. | # | Rule | From | Mechanism | Enforced? | |---|---|---|---|---| | 1 | Comments state only the current contract or a current maintainer constraint. No tickets, history, dates, measurements or review rationale. | both | `CommentPolicyTest` with a real Java **lexer** + normalized-hash baseline | **yes** | | 2 | A javadoc block stops at 30 lines. Over that, the knowledge moves to `docs/<subject>.md` and is linked in one line. | pos. 2 | same test; freeze today's 64 over-long blocks, ratchet down | **yes** | | 3 | No new mutable lifecycle state owners, and no new test seam in production source. | pos. 1 + my finding | cap `([Ff]orTest\|RaceHook)` at today's 49; review for the rest | **partly** | | 4 | The composition root does not grow, and a new static factory is not how you test wiring. | pos. 2 | cap `assembleAndStart` at 452 lines and `^ static ` on `Fleetd` at 54; ratchet both down | **partly** | | 5 | No new package cycle, and no widening of a recorded one. No new source-text test. | both | exact **edge** baseline replacing the broad `ignoreDependency` pairs; allow-list today's 8 source-reading tests | **yes** | ### Rulings on the three places the architects conflicted **Javadoc length limit — ADOPTED, over position 1's objection.** Position 1 rejects length limits. Its stated reason does not transfer: it argued a *class* can be long because it holds a schema or many tool handlers. There is no equivalent honest reason for a 235-line javadoc block (`config/ConfigRef.java:17`). And position 2's rule says *move* the content, not delete it, which answers position 1's real worry about losing concurrency invariants. **Mechanism — position 1's lexer wins over position 2's regex.** Both architects used regex classifiers and both reported false positives in their own output. A regex over Java source will flag string literals and text blocks. The gate must lex. **Scope of cleanup — position 1's "touched-code only" wins.** No bulk comment sweep. `MessageService` comments carry live lock and waiter invariants, and a sweep is how we would lose one. ### What is NOT enforced, stated plainly Rules 3 and 4 have caps, and **a cap is not the rule**. A number can stop the count growing; it cannot tell a good decomposition from a bad one. Position 2 said this about its own most important rule and refused to pretend otherwise. I am keeping that refusal in the decision: the judgement half of rules 3 and 4 is a review obligation, not a build check. ### Cost, not hidden The repo `CLAUDE.md` is **504 lines / 6,335 words / 40,219 bytes** today, and every session loads it. Position 2 costed its own proposal at ~34 lines, about +7%, and declined to call that free. I am capping the new section at **~20 lines**, because position 1 is right that the baseline format, the token list and the examples belong in the test and the reviewer skill, not in recurring context. ### Delivery order — #749 first 1. **#749** — the verified `PackageCyclesTest` javadoc-vs-code defect, found by position 1. It must be fixed before rule 5 lands, because rule 5 replaces the mechanism it documents. 2. **The 2 dead test names and 5 orphaned javadocs.** Comment-only edits, no behaviour risk. These are fixed, not frozen — they are already wrong today. 3. **The policy text** into the repo `CLAUDE.md`, with the "goes in the ADR" line corrected to `docs/<subject>.md`. **This project has no ADR directory**, which is why that line was unfollowable: it sent knowledge to a destination that does not exist. 4. **The gates**, as three separate units per position 1's split, so policy work is never hidden inside a refactor. Both architect panes are released after this comment.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#748
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?