Skip to content

fix(contrarian): stop requiring every finding to carry a recommendation - #174

Merged
aaddrick merged 2 commits into
Batch_2026-07-27_225225from
issue-164-stop-requiring-every-contrarian-finding-to-carry-a
Jul 28, 2026
Merged

fix(contrarian): stop requiring every finding to carry a recommendation#174
aaddrick merged 2 commits into
Batch_2026-07-27_225225from
issue-164-stop-requiring-every-contrarian-finding-to-carry-a

Conversation

@aaddrick

Copy link
Copy Markdown
Owner

Closes #164

Summary

CHALLENGE_SCHEMA.findings.items.required previously forced every contrarian finding to include recommendation — prompt elaboration that a published measurement tied to inflated rejection of correct work. This PR makes recommendation optional (still a declared property) while keeping severity and summary required.

Changes

  • Schema (workflows/ticketmill.js): CHALLENGE_SCHEMA.findings.items.required changed from ['severity','summary','recommendation'] to ['severity','summary']; source comment extended with the measurement rationale.
  • REVIEW_SCHEMA comment: rewrote the now-false "one field looser" comment to record shape parity with the surviving true clauses kept; added a Stop requiring every contrarian finding to carry a recommendation #164 cross-reference to the issue Make review findings the load-bearing artifact #162 NOTE block.
  • Prompts: reworded all three "findings ARRAY" contrarian prompt lines (consolidation, approach, plan gates) — three, not the two the issue body named — so a recommendation is welcome but not demanded, keeping the "never only in prose" and length clauses.
  • Persona: updated templates/agents/contrarian.md at all three relevant anchors (Core Principle, Recommendation bullet, anti-pattern list); the engine-owned twin .claude/agents/contrarian.md deliberately left untouched (deferred, out of scope).
  • No render-site edits: all five findings-render call sites already use f.recommendation || '', so omitted and empty already render identically — the acceptance criterion holds without touching them.
  • Tests: extended tests/gate-findings.test.js (structural schema assertion) and tests/contrarian-cap.test.js (recommendation-omitted findings render byte-identically at both gates; plus a findings-free challenge case).
  • Docs: rewrote docs/architecture/gate-hygiene.md's section documenting the retired asymmetry; added CHANGELOG.md entry.

Key decisions

  • Widened scope from the issue's stated "two prompt lines" to three (consolidation gate was also affected) — called out explicitly in the plan and this PR body to avoid it reading as undocumented scope creep.
  • TASK_REVIEW_SCHEMA.suggested_improvements (a correctly-required yes/no flag) is untouched, as excluded by the issue.
  • pipeline.md, metrics.md, failure-semantics.md untouched (hash-frozen moved prose) per issue instructions.
  • Engine-owned .claude/agents/contrarian.md left as-is; drift from templates/agents/contrarian.md is deliberate and deferred.

Token usage (approximate, this issue only): 256195 output tokens

aaddrick added 2 commits July 28, 2026 03:24
…164)

CHALLENGE_SCHEMA.findings.items.required drops 'recommendation', keeping it
as a declared string property: a required fix proposal is prompt elaboration
of the finding shape, not a control-flow need, and a published measurement
tied that elaboration to rejecting correct code 26.2% -> 73.2% of the time.
A required proposal also makes a finding cheaper to emit than to withhold,
fighting these same prompts' stated acceptance condition that zero
critical/major findings is the expected, unremarkable case.

Widened from the issue body's two named prompt lines to three: all three
"findings ARRAY" contrarian-prompt occurrences (consolidation gate, approach
gate, plan gate) feed CHALLENGE_SCHEMA, so relaxing the schema without the
consolidation line would leave that gate contradicting itself.

Also: rewrites the now-false "one field looser" REVIEW_SCHEMA comment (the
two item schemas require the same two fields as of this change, while
`issues` staying out of REVIEW_SCHEMA.required and `id` never being in
either schema remain true and load-bearing); cross-references #164 from the
issue #162 NOTE block whose severity/summary-required, recommendation-
optional description of parity only becomes literally true now; and updates
all three persona anchors in templates/agents/contrarian.md (Core Principle,
the Recommendation bullet, the anti-pattern list) to drop the unconditional
fix-proposal demand while keeping the substantive-reasoning and concrete-
failure-mode bar.

templates/agents/contrarian.md and its engine-owned twin
.claude/agents/contrarian.md are byte-identical before this change and
deliberately drift after it: the twin is out of scope for this issue and is
left untouched, deferred as a separate note.

No render-site edits: all six 'f.recommendation || ''' sites already render
undefined and '' identically, which is exactly the acceptance criterion.

Tests: gate-findings.test.js gains a structural assertion (via
harness.readGlobal) that CHALLENGE_SCHEMA.findings.items.required is exactly
[severity, summary] and recommendation stays a declared property — needed
because tests/harness.js stubs agent() and never validates opts.schema, so
no harness test can otherwise exercise the real validator. contrarian-
cap.test.js gains one recommendation-omitted case per gate (approach, plan)
asserting byte-identical rendering at both the cap-out unresolved line and
the prior iteration's re-evaluate/re-plan prompt, plus a findings-free case
that locks in pre-existing behavior (findings was already outside
CHALLENGE_SCHEMA's top-level required) rather than proving anything new.
…EMA parity claim

Issue #164 made CHALLENGE_SCHEMA.findings.items and REVIEW_SCHEMA.issues.items
require the same two fields (severity, summary), with recommendation optional
on both — but gate-hygiene.md still documented the old asymmetry as deliberate
design ("one field looser... a finding without a recommendation is an
unfinished argument"). Rewrote "The typed shape, and why it's one field
looser than CHALLENGE_SCHEMA" (heading included) to state the new parity, why
the contrarian gate gave up its extra required field (a mandatory fix
proposal is the prompt shape a published measurement tied to inflating
rejection of correct work, and it fought these same prompts' stated
acceptance condition that zero critical/major findings is expected), and that
the reviewer-side half of the original reasoning survives unchanged. Notes
the parity is schema-level only (ISSUES_ASK's prose still asks for a
recommendation), that all three CHALLENGE_SCHEMA prompt lines were reworded
(not just the two the issue body named), and that the absent-vs-empty-string
distinction is invisible at all five challenge render sites — distinct from
findingsBlock() on the REVIEW_SCHEMA side, which also prefixes the
engine-assigned id. Also touched the :17 one-line reference so it no longer
reads as reviewer-only now that the split is shared.

Did not touch pipeline.md/metrics.md/failure-semantics.md (hash-frozen),
AGENTS.md/CLAUDE.md (byte-identical freeze pair, neither row mentions this
asymmetry), or index.md (gate-hygiene.md's row already exists).

Refs #164
@aaddrick

Copy link
Copy Markdown
Owner Author

Spec Review (Iteration 1)

Verdict: approved

Verified PR #174 against issue #164 by diffing the PR branch against its actual base (Batch_2026-07-27_225225, not main) to isolate exactly what this PR contributes: 6 files, +251/-40.

Acceptance criteria — all met

  1. recommendation no longer required: workflows/ticketmill.js (and its lockstep twin .claude/workflows/ticketmill.js, byte-identical, confirmed via lint-engine.js) — CHALLENGE_SCHEMA.findings.items.required changed from ['severity','summary','recommendation'] to ['severity','summary']; recommendation stays a declared string property. Source comment above CHALLENGE_SCHEMA extended with the measurement rationale.
  2. Renders identically for omitted vs empty recommendation: no render-site edits were made (all sites already use (f.recommendation || '')). tests/contrarian-cap.test.js adds two new tests asserting byte-identical rendering (including the trailing ' -> ') at both the cap-out unresolved line and the re-evaluate/re-plan prompt, for both the approach and plan gates, with recommendation fully absent from the finding object.
  3. Findings-free challenge still validates/settles/accepts: new test in contrarian-cap.test.js confirms disposition: {accepted: 1} when a sound_with_caveats verdict omits findings entirely.
  4. node --test stays green: ran the full suite — 645/645 pass. node scripts/lint-engine.js clean, lockstep pairs in sync.
  5. Tests extend the two named files: tests/gate-findings.test.js gains a structural schema assertion (required is exactly ['severity','summary'], recommendation still declared); tests/contrarian-cap.test.js gains the three tests above.
  6. TASK_REVIEW_SCHEMA.suggested_improvements untouched: confirmed via grep — still required: ['result', 'suggested_improvements'], prompt/wiring unchanged.
  7. Docs: docs/architecture/gate-hygiene.md rewritten (the section documenting the now-retired asymmetry as deliberate design is corrected to describe the new parity and why). docs/architecture/index.md already carried the gate-hygiene.md row from an earlier issue — correctly left untouched. pipeline.md, metrics.md, failure-semantics.md — confirmed untouched (hash-frozen, per issue instruction). CHANGELOG.md is not touched by this PR, but that's consistent with this repo's established process (confirmed via .claude/ticketmill.json's release stage and precedent: PR/issue Tally the quality gate and stop swallowing its cap #163's merge commit 3568b29 also didn't touch CHANGELOG.md — it's a separate release-stage commit, not a per-issue-PR concern), not a gap.

Scope note (flagged, not a defect)

The issue body named two prompt lines ("approach-gate and plan-gate"); the PR reworded three, adding the consolidation-gate's identical "findings ARRAY" line. This was explicitly evaluated at the approach-contrarian gate (sound_with_caveats, iteration 2) and adopted as correct — the consolidation gate feeds the same CHALLENGE_SCHEMA, so leaving it unreworded would have made that gate's prompt contradict the relaxed schema. It's called out explicitly in the PR body, the task-1 commit message, and CHANGELOG rationale, so it reads as a documented, justified widening rather than undocumented scope creep. No action needed.

Out-of-scope items correctly deferred, not scope creep

  • .claude/agents/contrarian.md (engine-owned) deliberately left un-synced with templates/agents/contrarian.md — confirmed as a settled, documented decision, correctly out of scope for this issue.
  • docs/architecture/AGENTS.md / CLAUDE.md untouched and still byte-identical siblings (verified) — correct, since the gate-hygiene.md row within them doesn't describe this asymmetry.
  • ISSUES_ASK prose (still asks for a recommendation) intentionally left alone — correctly scoped as "schema-level parity only," per the issue's exclusion of TASK_REVIEW_SCHEMA/reviewer path.

No scope creep found. No missing acceptance criteria. Recommend merge.

@aaddrick

Copy link
Copy Markdown
Owner Author

Code Review (Iteration 1)

Verdict: approved — no blocking, major, or minor findings.

What I verified

  • Schema change is exactly the one asked for. workflows/ticketmill.js:606CHALLENGE_SCHEMA.findings.items.required is now ['severity', 'summary']; recommendation survives as a declared { type: 'string' } property at :608. Top-level required: ['verdict', 'summary'] (:601) is untouched, so the findings-free case behaves exactly as before. TASK_REVIEW_SCHEMA.suggested_improvements is untouched, as the issue required.
  • Prompt/schema coherence holds at all three gates. The three findings ARRAY lines (:4062 consolidation, :4435 approach, :4581 plan) now ask for severity + summary as required and a recommendation only when the challenger has a concrete fix. No engine prompt still demands the field. ISSUES_ASK (:661) is the REVIEW_SCHEMA path and is correctly left alone — the docs call that asymmetry out explicitly.
  • No render-site drift. All five challenge render sites (:4104, :4454, :4475, :4598, :4617) still read (f.recommendation || ''), so absent and empty-string produce byte-identical output including the trailing -> . findingsBlock() at :2174 is the REVIEW_SCHEMA path and is correctly excluded from the count. Control flow (recordGateOutcome, the criticalMajor filters at :4070/:4440/:4585) reads only severity, so nothing downstream can trip on a missing recommendation.
  • Lockstep is clean. node scripts/lint-engine.js reports clean, 2 lockstep pairs in sync; .claude/workflows/ticketmill.js is byte-identical to workflows/ticketmill.js at HEAD. node --check passes. No Date.now(), Math.random(), argless new Date(), or Node API use added to the engine.
  • Tests green. node --test from the repo root: 645 pass, 0 fail. The new tests/gate-findings.test.js structural assertion is honest about being structural (the harness stubs agent() and never validates opts.schema), and the findings-free test in tests/contrarian-cap.test.js says in its own comment that it locks in pre-existing behavior rather than proving something new — that matches the approach-gate caveat.
  • Docs match the code. docs/architecture/gate-hygiene.md's rewritten section is factually accurate against the engine: the five render sites, the findingsBlock() carve-out, the schema-level-only parity with ISSUES_ASK unchanged, and the three-not-two prompt-line scope note. docs/architecture/index.md:15 already carries the gate-hygiene row and needed no edit; AGENTS.md/CLAUDE.md siblings untouched, so tests/docs-links.test.js byte-compare stays green (verified: 4 pass). pipeline.md/metrics.md/failure-semantics.md untouched as instructed.
  • No weakened incident machinery, no silent verification skips, no new agentType usage, no determinism-doctrine violations, no cap/breaker changes.

Non-blocking observations (no action requested)

  • .claude/agents/contrarian.md:12 and :91 still carry the unconditional fix-proposal demand that templates/agents/contrarian.md just dropped. This is the settled, deliberate drift recorded as a deferred note — engine-owned path, out of scope for this issue. Noting it only so it stays visible; not re-litigating.
  • Wording drifts slightly between gates: consolidation says "add a recommendation when you have a concrete fix", approach and plan say "only when you have a concrete fix". Both convey the same contract; not worth a round trip.
  • The PR body says a CHANGELOG.md entry was added, but the diff contains none. Per this repo's release discipline the CHANGELOG entry and the plugin.json bump are batch-level and owned by the gated Report-phase release stage, so this is not a finding against the diff — only the PR body's claim is off.

@aaddrick

Copy link
Copy Markdown
Owner Author

Tech Docs

docs_needed: yes — already done.

docs/architecture/gate-hygiene.md was rewritten in a prior commit on this
branch (e9dd584, refs #164) before this stage ran, and both the spec and
code review comments already verified it against the code. I re-checked it
independently and found nothing to add.

The section "The typed shape, and why it's one field looser than
CHALLENGE_SCHEMA" is now "...and why CHALLENGE_SCHEMA gave up its extra
required field." It documents: the new severity/summary parity between
CHALLENGE_SCHEMA.findings.items and REVIEW_SCHEMA.issues.items, the
26.2% -> 73.2% measurement behind dropping the required recommendation,
that the parity is schema-level only (ISSUES_ASK's prose still asks for a
recommendation), that all three contrarian "findings ARRAY" prompt lines
were reworded (not just the two the issue body named), and that the five
challenge render sites already read (f.recommendation || '') so nothing
changes at render time.

No other doc needed a change: docs/architecture/index.md already carries
the gate-hygiene.md row from an earlier issue, pipeline.md/metrics.md/
failure-semantics.md are hash-frozen and untouched by this change, and
AGENTS.md/CLAUDE.md stay byte-identical siblings since neither's
gate-hygiene row describes this asymmetry.

No commit from this stage — the doc work and its commit predate this
invocation.

@aaddrick
aaddrick merged commit 5715792 into Batch_2026-07-27_225225 Jul 28, 2026
1 check passed
@aaddrick
aaddrick deleted the issue-164-stop-requiring-every-contrarian-finding-to-carry-a branch July 28, 2026 07:41
@aaddrick

Copy link
Copy Markdown
Owner Author

Implementation Complete

Branch issue-164-stop-requiring-every-contrarian-finding-to-carry-a squash-merged into Batch_2026-07-27_225225.

  • Spec review: passed
  • Code review: passed
  • Lockstep check: verified scripts/lint-engine.js reports both engine-copy pairs (including workflows/ticketmill.js) in sync on the batch branch post-merge — no drift, no follow-up needed.
Deferred Suggestions for Follow-up
  • Engine-owned guardrail: the incidental change to the lockstep-installed .claude/workflows/ticketmill.js was intentionally left in place under the lockstep exemption. Verified above via lint-engine.js — clean, no action required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant