.agents/skills/maintainer-review/references/evaluation-framework.md
Use this reference when an ioredis issue or PR claim is ambiguous, severity is disputed, or a technically correct PR may not justify permanent maintenance.
Treat validity, severity, and merge-worthiness as separate outputs. Also distinguish a Preliminary assessment, which may still require approved runtime evidence, from a final Maintainer decision. Do not label a provisional positive result as a verdict or final decision.
| Dimension | Question | Strong evidence |
|---|---|---|
| Claim validity | Does the exact behavior occur, and is the proposed cause correct? | Reproduction, failing focused test, or complete reachable path |
| Reachability | Can supported realistic inputs reach it? | Public API trace, real configuration, user report, or release comparison |
| Consequence | What fails, and is it silent or recoverable? | Observed output/error/state and downstream effect |
| Breadth | Which ioredis topology, protocol mode, command path, Node.js version, and Redis version are affected? | Explicit path and compatibility matrix |
| Frequency | Is it normal, intermittent, or pathological? | Repeats, deterministic preconditions, reports, or ioredis issue evidence |
| Need evidence | Is the exact scope demonstrated, merely plausible, already covered, or unsupported? | Same-scope user scenario, real-path reproduction, released compatibility requirement, violated supported contract, repeated demand, or broad consequential invariant |
| Unmet need | What user outcome cannot be achieved through supported behavior today, or what supported contract is violated? | Concrete scenario plus a trace showing why the closest existing path is insufficient or defective |
| Existing capability | Can configuration, composition, duplicated clients, callbacks, command transformers, extension points, or a caller-owned layer already satisfy the outcome? | Current release code, tests, docs, and an exact supported workflow |
| Compatibility | Is released API, export surface, TypeScript declaration, Redis protocol behavior, command result, or durable state changed? | Latest release comparison and contract inspection |
| Solution fit | Is the requested mechanism the best design and implementation layer? | Proposed solution compared with the strongest existing path and at least one narrower or more coherent alternative |
| Resource ownership | Can stale, failed, cancelled, or overlapping work mutate or clean up resources owned by surviving work? | Interleaving trace, attempt or generation ownership, and survivor assertions |
| Maintenance cost | What permanent complexity and review burden is added? | New branches/configuration, changed surface, tests, and remaining work |
Severity is consequence multiplied by realistic reach and frequency, reduced by recoverability. Do not raise it because prose is alarming or lower it because a diff is small.
Before calling a claim confirmed, answer:
built/, wrong worktree/imports, dependency drift, proxies, caches, runtime conditions, unavailable Redis/Docker/sandbox, authentication, ACLs, and service failures excluded?Use partially confirmed when the symptom is real but cause/reach/scope is wrong. Use unproven when decisive evidence is missing. Use contradicted only when evidence directly disproves the claim.
Issue reports often combine a desired outcome with a proposed API or implementation. Treat the proposed mechanism as a hypothesis. Confirm the unmet outcome or violated supported contract before evaluating how well the patch implements that mechanism.
Evidence from a linked issue applies only when the issue and PR share the same topology, protocol mode, connection mode, trigger, supported configuration, Redis version, and user outcome. A broad title, ordinary reference, Related to statement, or conceptual similarity is not enough. If an earlier change already resolved the concrete reported scenario, an adjacent extension starts with no inherited evidence of need.
Assign one status before deep implementation review:
Only Demonstrated need can support a merge-worthy code recommendation. Plausible but unproven maps to Needs evidence or Not worth completing, even when the patch is technically correct and its remaining fixes are bounded. Already covered and Unsupported normally map to closure or a simpler non-core alternative.
Before accepting an issue or recommending a PR, record:
| Question | Required evidence |
|---|---|
| What outcome is needed? | A concrete supported scenario stated without the proposed API or fix |
| What exists today? | The closest current-release API, configuration, composition, extension point, or caller-owned solution |
| Why is it insufficient? | An exact behavioral, compatibility, lifecycle, or operational constraint, not preference alone |
| What are the alternatives? | The proposed patch, the strongest existing path, and at least one no-code, narrower, or better-layer design |
| Why add a contract? | Practical benefit sufficient to justify public surface, runtime branches, cross-path tests, documentation, and long-term maintenance |
Classify the result:
Passing tests for a new implementation establish feasibility and correctness, not need. A fake socket, manually constructed Redis reply, mock, or synthetic fixture does not establish realistic Redis behavior, user reach, frequency, consequence, or demand. API symmetry and parity with an adjacent command, topology, or return type are design arguments, not need evidence. A technically coherent patch can still be Not worth completing when the motivating scenario is hypothetical, already supported, or better solved elsewhere.
Use the counterfactual maintainer test: if the PR did not already exist, would maintainers choose to file and implement the same work from the available evidence? Contributor effort lowers implementation cost but does not create product need or remove permanent maintenance cost.
When the need is not Demonstrated, inspect implementation only far enough to estimate contract, risk, and maintenance cost. Do not convert patch defects, missing tests, or documentation gaps into a request-changes disposition; those become merge blockers only after the need gate passes.
Choose one:
Ask only for evidence that could change the disposition.
Assess independently:
A PR can be correct but not merge-worthy because the need is negligible, the outcome is already supported through a reasonable existing mechanism, the real path is unchanged, equivalent paths remain inconsistent, the abstraction costs more than the benefit, or a simpler design exists at another layer.
Do not use implementation correctness, bounded remaining work, CI status, or contributor effort to upgrade a need that is only Plausible but unproven. Merge-worthiness is gated by demonstrated need, not by how close the patch is to completion.
Keep issue severity separate from Patch risk. A patch-induced regression, compatibility break, listener/resource leak, or maintenance hazard does not make the underlying issue more severe.
Make docs merge-blocking only when:
Keep optional discoverability/completeness non-blocking. Do not downgrade a code recommendation solely for optional docs or include optional docs in a required-action paragraph.
Apply this section when a change adds validation, fail-fast behavior, cleanup, retry, interruption, background work, streaming, or concurrency.
Apply this section before a positive assessment whenever lifecycle work crosses an await, callback, event, deferred completion, retry, reconnect, cancellation, or shared resource boundary. Sequential correctness is insufficient because the patch can improve isolated cleanup while introducing cross-attempt teardown.
Use a two-operation interleaving matrix during desk review:
| Ordering | Required question |
|---|---|
A pending -> B starts -> A fails -> B succeeds | Can A's cleanup remove or revert anything B needs? |
A pending -> B starts -> B fails -> A succeeds | Can B's cleanup leave A successful but non-functional? |
A succeeds -> B starts -> stale A completion | Can stale A overwrite B's newer state or generation? |
| setup -> close/cancel -> late completion | Can late work resurrect listeners, state, tasks, or connections after teardown? |
For each ordering:
An unscoped finally, catch, close handler, cancellation callback, or rollback that mutates shared state after a suspension point is merge-blocking when another operation can still own or use that state.
Start with the strongest existing supported path, then test at least one additional alternative against the proposed patch. Do not complete a positive review without this comparison.
Require an explicit issue link, same reproduction, same violated invariant, or materially overlapping runtime path before grouping candidates.
| Criterion | Question |
|---|---|
| Need | Does a concrete user outcome remain unmet, or supported behavior remain defective, after tracing existing functionality? |
| Existing capability | Could every candidate be avoided by configuration, composition, an extension point, or a better Redis server, connector, command transformer, or caller-owned solution? |
| Coverage | Whole confirmed issue, useful subset, or adjacent problem? |
| Correctness | Real path and meaningful boundaries? |
| Placement | Owning shared layer? |
| Tests | Base failure reproduced and approaches distinguished? |
| Compatibility | Released APIs, TypeScript declarations, state, Redis protocol behavior, command results, topologies, and Node/Redis versions? |
| Complexity | Permanent branches, abstractions, configuration, coupling? |
| Readiness | Mergeable now or bounded focused work? |
| Reuse | Exact tests or ideas worth transferring? |
Choose one portfolio action:
Do not issue independent approvals for overlapping candidates. State the action for every active PR.
Write drafts in English. Produce one when recommending closure, more evidence, focused changes, superseding, or choosing among competing PRs.
Keep it polite, direct, complete, and usually 60-160 words in one to three short paragraphs:
Do not include internal severity labels, speculate about authorship/intent, repeat the full review, or soften the requested action until it is unclear.
Thanks for taking the time to investigate this. I traced the reported case through <path or behavior>, and <decisive finding>. In the supported path, <practical result>, so the added complexity is not justified by the demonstrated impact.
I am going to close this <issue/PR>. If you can provide <specific reproduction or evidence that would change the decision>, we can revisit the underlying problem with that narrower scope.
Thanks for the contribution. The underlying issue is valid, and this approach is directionally reasonable. Before we can merge it, please address the following points: <bounded required changes>.
These changes are needed because <contract, lifecycle, compatibility, or test reason>. Once they are covered with a regression test that fails on the base and passes on the updated branch, the PR should be ready for another review.
Adapt these templates to evidence. Do not use them as filler.
Thanks for the contribution. I traced the underlying use case through <existing API or workflow>, which already supports <desired outcome and relevant limits>. The proposed change adds <new contract or complexity>, but the issue does not demonstrate a concrete supported case that the existing approach cannot handle.
I am going to close this <issue/PR> for now. If you can provide <specific scenario showing the existing approach is insufficient>, we can revisit the unmet need and choose the narrowest appropriate design from that evidence.
Use Maintainer decision for a concluded review. Use Preliminary assessment when a desk review is tentatively positive but a decision-relevant runtime concern remains. Verdict is intentionally avoided in the report headings because it does not communicate whether the result is provisional or final.
## Preliminary assessment
<Tentative issue or PR assessment based on desk review only.>
## Static evidence
- <decisive code-path or test-inspection evidence>
- <what remains uncertain at runtime>
## Proposed runtime probe
- Concern: <the uncertainty that could change the decision>
- Probe: <smallest exact execution path>
- Control: <base, release, or known-good comparison when relevant>
- Scope: <local ioredis setup, Redis version/topology, mutation, or cleanup implications>
## Approval request
<Ask whether to run this exact probe. Do not present a final positive recommendation yet.>
## Maintainer decision
<Real/partial/unproven/contradicted, severity, and disposition.>
## Evidence
- <decisive evidence>
- <scope or uncertainty>
## Existing capability and alternatives
<Closest supported path, why it is or is not sufficient, and the preferred design alternative.>
## Recommendation
<Prioritize, accept low priority, narrow, request evidence, or close.>
## Maintainer comment draft
<Include for closure or an evidence request.>
## Maintainer decision
<Need, practical impact, and merge-worthiness.>
- Need evidence: <Demonstrated / Plausible but unproven / Already covered / Unsupported>
- Code recommendation: <code disposition>
- Repository readiness: <one allowed status; only when useful for a merge-worthy recommendation>
## Evidence
- <runtime/code-path result>
- <test/compatibility result>
## Existing capability and alternatives
<Closest supported path, why the demonstrated scenario cannot use it or remains defective, and why this patch is preferable to no code change or a narrower design.>
## Issue impact
- Validity: <claim validity>
- Severity: <underlying issue severity>
- Reach: <realistic reach>
## Patch risk
<Only meaningful patch-induced risk.>
## PR quality
- Solution fit: <assessment>
- Tests: <assessment>
- Remaining effort: <bounded/unbounded and why>
## Recommendation
<Merge, focused changes, simpler replacement, or close.>
## Maintainer comment draft
<Only when closure, evidence, or changes should be requested.>
## Maintainer decision
<Issue validity, severity, and preferred implementation path.>
## Open PR comparison
| PR | Approach | Correctness | Tests | Compatibility/complexity | Readiness |
| ---- | -------- | ----------- | ----- | ------------------------ | --------- |
| #... | ... | ... | ... | ... | ... |
## Recommendation
<Select one, request focused changes, combine exact pieces, replace all, or merge none.> <State the action for every other active candidate.>
## Maintainer comment drafts
<One draft for each PR that should be closed, changed, or superseded.>