Productize agent-consensus as a standalone library - #4
Conversation
Summary by CodeRabbit
WalkthroughThe project is converted into a typed ChangesConsensus library
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConsensusEngine
participant Participants
participant ConsensusResult
Caller->>ConsensusEngine: run(prompt)
ConsensusEngine->>Participants: invoke bounded async responders
Participants-->>ConsensusEngine: validated responses or isolated failures
ConsensusEngine->>ConsensusResult: aggregate weighted outcomes
ConsensusResult-->>Caller: return status, choice, tallies, and audit fields
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 19-20: Pin every actions/checkout@v6 and actions/setup-python@v6
reference in .github/workflows/ci.yml to its corresponding full immutable commit
SHA, including the sites at lines 19-20, 44-45, and 58-59, while preserving the
existing action configuration and enabling Dependabot maintenance.
In `@agent_consensus/core.py`:
- Around line 54-65: Extract the shared threshold and minimum-successful
validation from _validate_decision_settings and ConsensusConfig.__post_init__
into one reusable function in models.py. Update both callers, including
evaluate_votes and ConsensusConfig.__post_init__, to use that function while
preserving their existing validation behavior and error messages.
- Line 1: Consolidate the duplicated threshold and min_successful validation by
extracting the shared checks from ConsensusConfig.__post_init__ into an exported
module-level validator in models.py, such as
_validate_threshold_and_min_successful. Remove _validate_decision_settings from
core.py, import and call the shared validator wherever the core path currently
performs those checks, and preserve the existing validation behavior.
In `@agent_consensus/models.py`:
- Around line 162-173: The __post_init__ validation duplicates
_validate_decision_settings. Consolidate threshold and min_successful validation
into one shared validator, then have both __post_init__ and
_validate_decision_settings reuse it while preserving the existing
ConfigurationError messages and validation behavior.
- Around line 34-82: Update ParticipantResponse to resolve the mismatch between
its frozen dataclass hash behavior and the MappingProxyType metadata field:
either mark metadata with hash=False while preserving equality semantics, or
explicitly set ParticipantResponse.__hash__ = None if instances should remain
unhashable. Ensure hashing no longer raises TypeError and the chosen
hash/equality contract is consistent.
In `@docs/CONSENSUS_ALGORITHMS.md`:
- Around line 16-19: Update the example’s normalizer lambda to trim and collapse
whitespace before performing the case-insensitive alias lookup, so inputs such
as “ APPROVED ” resolve to “approve” while preserving the existing fallback
behavior.
In `@docs/PRODUCTIZATION.md`:
- Line 315: Update the CI/public-installation risk statement in
PRODUCTIZATION.md to remove the contradictory claim that no real CI run exists,
since a GitHub-hosted CI run is documented elsewhere. Narrow the statement to
the missing public-package installation evidence or explicitly qualify the
remaining validation as owner-controlled release validation.
In `@docs/RELEASING.md`:
- Around line 40-42: Update the release workflow guidance to grant the
publishing job only the permissions required by its selected artifact flow,
retaining id-token: write and avoiding checkout or broader read scopes unless
explicitly needed. Replace the mutable pypa/gh-action-pypi-publish@release/v1
reference with a reviewed full commit SHA directly in the documented release
procedure.
In `@README.md`:
- Line 139: Update the README sentence describing the default threshold to
hyphenate “two-thirds,” preserving the existing wording about all configured
participant weight.
In `@tests/test_consensus.py`:
- Around line 188-193: Update
test_result_is_json_serializable_with_json_metadata to exercise the metadata
serialization path by constructing the vote/response or ConsensusResult fixture
with JSON-compatible metadata, then assert that metadata is preserved in both
to_dict() and json.dumps() output. If the current API cannot attach metadata,
rename the test to accurately describe the behavior it verifies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5680b1d0-35a8-4845-a073-73c8e0995531
📒 Files selected for processing (42)
.github/CODEOWNERS.github/dependabot.yml.github/workflows/ci.yml.gitignoreCHANGELOG.mdCITATION.cffCODE_OF_CONDUCT.mdCONTRIBUTING.mdLICENSEMANIFEST.inNOTICEREADME.mdROADMAP.mdSECURITY.mdTRADEMARKS.mdagent_consensus/__init__.pyagent_consensus/core.pyagent_consensus/errors.pyagent_consensus/models.pyagent_consensus/multi_ai_consensus.pyagent_consensus/py.typeddocs/API_REFERENCE.mddocs/CONSENSUS_ALGORITHMS.mddocs/GETTING_STARTED.mddocs/LICENSING.mddocs/PRODUCTIZATION.mddocs/RELEASING.mdexamples/01_basic_consensus.pyexamples/02_conflict_resolution.pyexamples/03_fault_tolerance.pyexamples/04_monitoring.pyexamples/05_multi_proposal.pypyproject.tomlpytest.inirequirements-dev.txtrequirements-test.txtsetup.pytests/conftest.pytests/test_consensus.pytests/test_coordination.pytests/test_engine.pytests/test_package.py
💤 Files with no reviewable changes (5)
- setup.py
- pytest.ini
- tests/test_coordination.py
- requirements-test.txt
- tests/conftest.py
| - uses: actions/checkout@v6 | ||
| - uses: actions/setup-python@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Workflow action refs =="
rg -n "uses:\s*actions/(checkout|setup-python)@" .github/workflows/ci.yml || true
echo
echo "== Relevant workflow sections =="
nl -ba .github/workflows/ci.yml | sed -n '1,90p'
echo
echo "== Tag state from GitHub refs =="
python3 - <<'PY'
import json, os, urllib.request
for tag in ["checkout", "setup-python"]:
u = f"https://api.github.com/repos/actions/{tag}/git/refs/tags/v6"
req = urllib.request.Request(u, headers={
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
"User-Agent": "code-rabbit-review"
})
print(f"\n--- refs/tags/v6 for {tag} ---")
with urllib.request.urlopen(req, timeout=20) as r:
data = json.load(r)
print(json.dumps(data, indent=2))
if data.get("object", {}).get("type") == "tag":
tag_obj = data["object"]["sha"]
print(f"\nFollowed tag object {tag_obj} ...")
req2 = urllib.request.Request(
f"https://api.github.com/repos/actions/{tag}/git/tags/{tag_obj}",
headers={
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
"User-Agent": "code-rabbit-review"
}
)
with urllib.request.urlopen(req2, timeout=20) as r2:
print(json.dumps(json.load(r2), indent=2))
PYRepository: Deathcharge/agent-consensus
Length of output: 500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant workflow sections =="
awk '{printf "%6d:%s\n", NR, $0}' .github/workflows/ci.yml | sed -n '1,80p'
echo
echo "== Action refs summary =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path(".github/workflows/ci.yml")
text=p.read_text()
for m in re.finditer(r'^\s*-\s*uses:\s*([^\s#]+)', text, flags=re.M):
print(f"{text[:m.start()].count(chr(10))+1}: {m.group(1)}")
PY
echo
echo "== Tag state from GitHub refs =="
python3 - <<'PY'
import json, urllib.request
for tag in ["checkout", "setup-python"]:
url = f"https://api.github.com/repos/actions/{tag}/git/refs/tags/v6"
req = urllib.request.Request(url, headers={
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
"User-Agent": "code-rabbit-review"
})
print(f"\n--- {url} ---")
with urllib.request.urlopen(req, timeout=20) as r:
data = json.load(r)
print(data.get("sha"), data.get("ref"), data.get("object"))
obj_type = data.get("object", {}).get("type")
if obj_type == "tag":
tag_obj = data["object"]["sha"]
req2 = urllib.request.Request(
f"https://api.github.com/repos/actions/{tag}/git/tags/{tag_obj}",
headers={
"Accept": "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
"User-Agent": "code-rabbit-review"
}
)
with urllib.request.urlopen(req2, timeout=20) as r2:
tag_data = json.load(r2)
print("tag object -> commit:", tag_data.get("object", {}).get("type"), tag_data.get("object", {}).get("sha"))
PYRepository: Deathcharge/agent-consensus
Length of output: 5459
Pin GitHub Actions to immutable commit SHAs.
.github/workflows/ci.yml uses six mutable major-version action refs, and the workflow installs dependencies, checks code, runs tests, builds packages, executes examples/*.py, and imports agent_consensus. Pin each actions/checkout@v6 and actions/setup-python@v6 reference to a full commit SHA, with full SHA references for Dependabot to maintain.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 19-19: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
📍 Affects 1 file
.github/workflows/ci.yml#L19-L20(this comment).github/workflows/ci.yml#L44-L45.github/workflows/ci.yml#L58-L59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 19 - 20, Pin every actions/checkout@v6
and actions/setup-python@v6 reference in .github/workflows/ci.yml to its
corresponding full immutable commit SHA, including the sites at lines 19-20,
44-45, and 58-59, while preserving the existing action configuration and
enabling Dependabot maintenance.
Source: Linters/SAST tools
| @@ -0,0 +1,317 @@ | |||
| """Deterministic vote evaluation and bounded asynchronous fan-out.""" | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Threshold/min_successful validation is duplicated across two files. _validate_decision_settings in agent_consensus/core.py and ConsensusConfig.__post_init__ in agent_consensus/models.py implement byte-for-byte identical checks; the shared invariant should live in one place so the two can't silently drift apart on future edits.
agent_consensus/core.py#L54-65: remove the local_validate_decision_settingsand import a single shared validator fromagent_consensus/models.pyinstead.agent_consensus/models.py#L162-173: extract the threshold/min_successfulchecks into a module-level function (e.g._validate_threshold_and_min_successful) thatConsensusConfig.__post_init__calls, and export it for reuse bycore.py.
♻️ Proposed consolidation
# agent_consensus/models.py
+def _validate_threshold_and_min_successful(threshold: float, min_successful: int) -> None:
+ if (
+ isinstance(threshold, bool)
+ or not isinstance(threshold, (int, float))
+ or not math.isfinite(threshold)
+ or not 0 < threshold <= 1
+ ):
+ raise ConfigurationError("threshold must be greater than 0 and at most 1")
+ if isinstance(min_successful, bool) or not isinstance(min_successful, int):
+ raise ConfigurationError("min_successful must be an integer")
+ if min_successful < 1:
+ raise ConfigurationError("min_successful must be at least 1")
+
+
`@dataclass`(frozen=True)
class ConsensusConfig:
...
def __post_init__(self) -> None:
- if (
- isinstance(self.threshold, bool)
- or not isinstance(self.threshold, (int, float))
- or not math.isfinite(self.threshold)
- or not 0 < self.threshold <= 1
- ):
- raise ConfigurationError("threshold must be greater than 0 and at most 1")
- if isinstance(self.min_successful, bool) or not isinstance(self.min_successful, int):
- raise ConfigurationError("min_successful must be an integer")
- if self.min_successful < 1:
- raise ConfigurationError("min_successful must be at least 1")
+ _validate_threshold_and_min_successful(self.threshold, self.min_successful)# agent_consensus/core.py
-def _validate_decision_settings(threshold: float, min_successful: int) -> None:
- if (
- isinstance(threshold, bool)
- or not isinstance(threshold, (int, float))
- or not math.isfinite(threshold)
- or not 0 < threshold <= 1
- ):
- raise ConfigurationError("threshold must be greater than 0 and at most 1")
- if isinstance(min_successful, bool) or not isinstance(min_successful, int):
- raise ConfigurationError("min_successful must be an integer")
- if min_successful < 1:
- raise ConfigurationError("min_successful must be at least 1")
+from .models import _validate_threshold_and_min_successful as _validate_decision_settings🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent_consensus/core.py` at line 1, Consolidate the duplicated threshold and
min_successful validation by extracting the shared checks from
ConsensusConfig.__post_init__ into an exported module-level validator in
models.py, such as _validate_threshold_and_min_successful. Remove
_validate_decision_settings from core.py, import and call the shared validator
wherever the core path currently performs those checks, and preserve the
existing validation behavior.
| def _validate_decision_settings(threshold: float, min_successful: int) -> None: | ||
| if ( | ||
| isinstance(threshold, bool) | ||
| or not isinstance(threshold, (int, float)) | ||
| or not math.isfinite(threshold) | ||
| or not 0 < threshold <= 1 | ||
| ): | ||
| raise ConfigurationError("threshold must be greater than 0 and at most 1") | ||
| if isinstance(min_successful, bool) or not isinstance(min_successful, int): | ||
| raise ConfigurationError("min_successful must be an integer") | ||
| if min_successful < 1: | ||
| raise ConfigurationError("min_successful must be at least 1") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate threshold/min_successful validation logic.
Identical to ConsensusConfig.__post_init__ in agent_consensus/models.py (Lines 162-173). evaluate_votes needs standalone threshold/min_votes validation since it doesn't build a ConsensusConfig, but the check itself should be a single shared function (e.g. exposed from models.py, since core.py already depends on it) rather than copy-pasted, so the two can't silently drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent_consensus/core.py` around lines 54 - 65, Extract the shared threshold
and minimum-successful validation from _validate_decision_settings and
ConsensusConfig.__post_init__ into one reusable function in models.py. Update
both callers, including evaluate_votes and ConsensusConfig.__post_init__, to use
that function while preserving their existing validation behavior and error
messages.
| @dataclass(frozen=True) | ||
| class ParticipantResponse: | ||
| """A participant's explicit vote and optional supporting content. | ||
|
|
||
| ``choice`` is the only field used to calculate consensus. The library does | ||
| not pretend to infer semantic agreement from ``content``. | ||
| """ | ||
|
|
||
| choice: str | ||
| content: str = "" | ||
| confidence: float | None = None | ||
| tokens_used: int | None = None | ||
| metadata: Mapping[str, Any] = field(default_factory=dict) | ||
|
|
||
| def __post_init__(self) -> None: | ||
| if not isinstance(self.choice, str) or not self.choice.strip(): | ||
| raise ResponseValidationError("choice must be a non-empty string") | ||
| if len(self.choice) > MAX_CHOICE_CHARACTERS: | ||
| raise ResponseValidationError( | ||
| f"choice cannot exceed {MAX_CHOICE_CHARACTERS} characters" | ||
| ) | ||
| if not isinstance(self.content, str): | ||
| raise ResponseValidationError("content must be a string") | ||
| if self.confidence is not None and ( | ||
| isinstance(self.confidence, bool) | ||
| or not isinstance(self.confidence, (int, float)) | ||
| or not math.isfinite(self.confidence) | ||
| or not 0.0 <= self.confidence <= 1.0 | ||
| ): | ||
| raise ResponseValidationError("confidence must be between 0 and 1") | ||
| if self.tokens_used is not None and ( | ||
| isinstance(self.tokens_used, bool) | ||
| or not isinstance(self.tokens_used, int) | ||
| or self.tokens_used < 0 | ||
| ): | ||
| raise ResponseValidationError("tokens_used must be a non-negative integer") | ||
| if not isinstance(self.metadata, Mapping): | ||
| raise ResponseValidationError("metadata must be a mapping") | ||
| object.__setattr__(self, "metadata", MappingProxyType(dict(self.metadata))) | ||
|
|
||
| def to_dict(self) -> dict[str, Any]: | ||
| """Return a JSON-compatible shape when metadata values are compatible.""" | ||
| return { | ||
| "choice": self.choice, | ||
| "content": self.content, | ||
| "confidence": self.confidence, | ||
| "tokens_used": self.tokens_used, | ||
| "metadata": dict(self.metadata), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate models.py =="
fd -a 'models\.py$' . | sed 's#^\./##'
echo "== inspect agent_consensus/models.py relevant lines =="
sed -n '1,220p' agent_consensus/models.py | cat -n
echo "== behavioral probe: dataclass hash with MappingProxyType metadata =="
python3 - <<'PY'
import math
from dataclasses import dataclass, field
from collections.abc import Mapping
from types import MappingProxyType
MAX_CHOICE_CHARACTERS = 80
`@dataclass`(frozen=True)
class ParticipantResponse:
choice: str
content: str = ""
confidence: float | None = None
tokens_used: int | None = None
metadata: Mapping[str, object] = field(default_factory=dict)
def __post_init__(self):
if not isinstance(self.choice, str) or not self.choice.strip():
raise Exception("choice must be a non-empty string")
if len(self.choice) > MAX_CHOICE_CHARACTERS:
raise Exception(f"choice cannot exceed {MAX_CHOICE_CHARACTERS} characters")
if not isinstance(self.content, str):
raise Exception("content must be a string")
if self.confidence is not None and (
isinstance(self.confidence, bool)
or not isinstance(self.confidence, (int, float))
or not math.isfinite(self.confidence)
or not 0.0 <= self.confidence <= 1.0
):
raise Exception("confidence must be between 0 and 1")
if self.tokens_used is not None and (
isinstance(self.tokens_used, bool)
or not isinstance(self.tokens_used, int)
or self.tokens_used < 0
):
raise Exception("tokens_used must be a non-negative integer")
if not isinstance(self.metadata, Mapping):
raise Exception("metadata must be a mapping")
object.__setattr__(self, "metadata", MappingProxyType(dict(self.metadata)))
r = ParticipantResponse(choice="A")
print("hasattr __hash__:", hasattr(r, "__hash__") and r.__hash__ is not None)
if hasattr(r, "__hash__") and r.__hash__ is not None:
try:
print("hash(r) returns:", hash(r))
except Exception as exc:
print("hash(r) raised:", type(exc).__name__, str(exc))
else:
print("r is explicitly unhashable")
try:
s = {r}
except Exception as exc:
print("set{r} raised:", type(exc).__name__, str(exc))
d = {r: "cached"}
print("dict key {r: 'cached'} accepted, len:", len(d))
try:
print("hash(d[{r}]) returns:", hash(r))
except Exception as exc:
print("hash(d[{r}]) raised:", type(exc).__name__, str(exc))
PYRepository: Deathcharge/agent-consensus
Length of output: 10082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python version =="
python3 --version
echo "== behavioral probe with legacy-compatible annotation =="
python3 - <<'PY'
import math
from dataclasses import dataclass, field
from collections.abc import Mapping
from types import MappingProxyType
MAX_CHOICE_CHARACTERS = 80
`@dataclass`(frozen=True)
class ParticipantResponse:
choice = None
content = None
confidence = None
tokens_used = None
metadata = None
def __init__(self, choice, content="", confidence=None, tokens_used=None, metadata=None):
self.choice = choice
self.content = content
if content is None:
self.content = ""
self.confidence = confidence
self.tokens_used = tokens_used
self.metadata = metadata
self.__post_init__()
def __post_init__(self):
if not isinstance(self.choice, str) or not self.choice.strip():
raise Exception("choice must be a non-empty string")
if len(self.choice) > MAX_CHOICE_CHARACTERS:
raise Exception(f"choice cannot exceed {MAX_CHOICE_CHARACTERS} characters")
if not isinstance(self.content, str):
raise Exception("content must be a string")
if self.confidence is not None and (
isinstance(self.confidence, bool)
or not isinstance(self.confidence, (int, float))
or not math.isfinite(self.confidence)
or not 0.0 <= self.confidence <= 1.0
):
raise Exception("confidence must be between 0 and 1")
if self.tokens_used is not None and (
isinstance(self.tokens_used, bool)
or not isinstance(self.tokens_used, int)
or self.tokens_used < 0
):
raise Exception("tokens_used must be a non-negative integer")
if not isinstance(self.metadata, Mapping):
raise Exception("metadata must be a mapping")
object.__setattr__(self, "metadata", MappingProxyType(dict(self.metadata)))
def __eq__(self, other):
if isinstance(other, ParticipantResponse):
return self.choice == other.choice \
and self.content == other.content \
and self.confidence == other.confidence \
and self.tokens_used == other.tokens_used \
and self.metadata == other.metadata
return False
def __hash__(self):
return hash((self.choice, self.content, self.confidence, self.tokens_used, self.metadata))
r = ParticipantResponse(choice="A")
print("metadata type:", type(r.metadata).__name__)
print("hasattr __hash__:", hasattr(r, "__hash__") and r.__hash__ is not None)
try:
print("hash(r) returns:", hash(r))
except Exception as exc:
print("hash(r) raised:", type(exc).__name__, str(exc))
try:
s = {r}
print("set{r} accepted, len:", len(s))
except Exception as exc:
print("set{r} raised:", type(exc).__name__, str(exc))
try:
d = {r: "cached"}
print("dict key {r: 'cached'} accepted, len:", len(d))
print("hash again returns:", hash(r))
except Exception as exc:
print("dict key {r: 'cached'} raised:", type(exc).__name__, str(exc))
PYRepository: Deathcharge/agent-consensus
Length of output: 477
Keep ParticipantResponse hashable, or make it explicitly unhashable.
@dataclass(frozen=True) with default equality generates __hash__, but the stored metadata: MappingProxyType(dict(...)) field makes hashing any ParticipantResponse instance raise TypeError, so ParticipantResponse cannot be used as a set/dict key. Either exclude metadata from the generated hash with field(hash=False) if metadata values should not affect equality, or set __hash__ = None if the response object is intentionally not hashable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent_consensus/models.py` around lines 34 - 82, Update ParticipantResponse
to resolve the mismatch between its frozen dataclass hash behavior and the
MappingProxyType metadata field: either mark metadata with hash=False while
preserving equality semantics, or explicitly set ParticipantResponse.__hash__ =
None if instances should remain unhashable. Ensure hashing no longer raises
TypeError and the chosen hash/equality contract is consistent.
| def __post_init__(self) -> None: | ||
| if ( | ||
| isinstance(self.threshold, bool) | ||
| or not isinstance(self.threshold, (int, float)) | ||
| or not math.isfinite(self.threshold) | ||
| or not 0 < self.threshold <= 1 | ||
| ): | ||
| raise ConfigurationError("threshold must be greater than 0 and at most 1") | ||
| if isinstance(self.min_successful, bool) or not isinstance(self.min_successful, int): | ||
| raise ConfigurationError("min_successful must be an integer") | ||
| if self.min_successful < 1: | ||
| raise ConfigurationError("min_successful must be at least 1") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate threshold/min_successful validation logic.
This is byte-for-byte identical to _validate_decision_settings in agent_consensus/core.py (Lines 54-65). Two independent copies of the same invariant will drift silently if one is updated without the other. See paired comment in agent_consensus/core.py; consolidated proposal at the end of this review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent_consensus/models.py` around lines 162 - 173, The __post_init__
validation duplicates _validate_decision_settings. Consolidate threshold and
min_successful validation into one shared validator, then have both
__post_init__ and _validate_decision_settings reuse it while preserving the
existing ConfigurationError messages and validation behavior.
| ```python | ||
| def simple_majority(votes): | ||
| for_count = sum(1 for v in votes.values() if v == "for") | ||
| against_count = sum(1 for v in votes.values() if v == "against") | ||
| total = for_count + against_count | ||
| return for_count > total / 2 | ||
| aliases = {"approved": "approve", "yes": "approve"} | ||
| normalizer = lambda choice: aliases.get(choice.casefold(), choice.casefold()) | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize whitespace before applying aliases.
The example does not match the documented normalization behavior: " APPROVED " will not resolve to "approve" because the alias lookup occurs before trimming and whitespace collapsing.
Proposed fix
-aliases = {"approved": "approve", "yes": "approve"}
-normalizer = lambda choice: aliases.get(choice.casefold(), choice.casefold())
+aliases = {"approved": "approve", "yes": "approve"}
+normalizer = lambda choice: aliases.get(
+ " ".join(choice.split()).casefold(),
+ " ".join(choice.split()).casefold(),
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```python | |
| def simple_majority(votes): | |
| for_count = sum(1 for v in votes.values() if v == "for") | |
| against_count = sum(1 for v in votes.values() if v == "against") | |
| total = for_count + against_count | |
| return for_count > total / 2 | |
| aliases = {"approved": "approve", "yes": "approve"} | |
| normalizer = lambda choice: aliases.get(choice.casefold(), choice.casefold()) | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/CONSENSUS_ALGORITHMS.md` around lines 16 - 19, Update the example’s
normalizer lambda to trim and collapse whitespace before performing the
case-insensitive alias lookup, so inputs such as “ APPROVED ” resolve to
“approve” while preserving the existing fallback behavior.
| - A participant that performs blocking synchronous work can stall the shared event loop; adapters | ||
| must use non-blocking clients or explicitly move blocking work to a bounded thread executor. | ||
| - Result metadata can be non-JSON or sensitive because it is caller-owned. | ||
| - No real CI run or public installation evidence exists until the owner exercises external gates. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the CI/public-installation risk statement.
Line 270 records a passed GitHub-hosted CI run, so “No real CI run” at Line 315 contradicts this record. Narrow the statement to missing public-package installation evidence or explicitly qualify it as owner-controlled release validation.
Proposed wording
-No real CI run or public installation evidence exists until the owner exercises external gates.
+No public package installation evidence exists until the owner exercises the publication gates;
+GitHub-hosted CI has already passed as recorded above.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - No real CI run or public installation evidence exists until the owner exercises external gates. | |
| No public package installation evidence exists until the owner exercises the publication gates; | |
| GitHub-hosted CI has already passed as recorded above. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/PRODUCTIZATION.md` at line 315, Update the CI/public-installation risk
statement in PRODUCTIZATION.md to remove the contradictory claim that no real CI
run exists, since a GitHub-hosted CI run is documented elsewhere. Narrow the
statement to the missing public-package installation evidence or explicitly
qualify the remaining validation as owner-controlled release validation.
| The publishing job should receive only `id-token: write`, consume artifacts built by an unprivileged | ||
| job, and use `pypa/gh-action-pypi-publish@release/v1`. Pin third-party actions to reviewed commit | ||
| SHAs when the owner establishes the release workflow. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status =="
git status --short
echo "== docs/RELEASING.md snippets around Lines 30-60 =="
if [ -f docs/RELEASING.md ]; then
sed -n '1,120p' docs/RELEASING.md | nl -ba | sed -n '30,70p'
else
echo "docs/RELEASING.md not found"
fi
echo "== workflow/action references in repository =="
rg -n "permissions:|id-token:|contents:|actions:|artifacts|pypa/gh-action-pypi-publish|release/v1|uses:" -S . --glob '!node_modules/**' --glob '!dist/**' --glob '!.git/**' --glob '*.yml' --glob '*.yaml' --glob '*.md' ... 2>/dev/null | head -200Repository: Deathcharge/agent-consensus
Length of output: 274
🌐 Web query:
GitHub Actions workflow syntax permissions id-token actions read contents read download-artifact permissions
💡 Result:
In GitHub Actions workflows, the permissions key is used to define the access rights granted to the GITHUB_TOKEN for a job or an entire workflow [1][2][3]. If you specify any permissions, all unspecified permissions are set to none [2][3]. id-token permission The id-token permission is used exclusively to request an OpenID Connect (OIDC) JSON Web Token (JWT) for authentication with external cloud providers [4][5][6]. It must be set to write if you need to fetch this token [5][6]. Setting it to read is invalid and will be rejected, as there are no resources to read [7]. Effectively, id-token: none and id-token: read (if it were allowed) provide the same level of access [7]. actions: read and contents: read These permissions control access to GitHub API operations: - actions: read: Allows the workflow to read workflow run and artifact data [4]. - contents: read: Allows the workflow to read repository content, which is typically required to checkout code using actions/checkout [5][2]. download-artifact permissions By default, the actions/download-artifact action can download artifacts from the current workflow run without requiring additional permissions, provided it is part of the same workflow [8][9]. If you need to download artifacts from a different workflow run or repository, you must use a personal access token (PAT) with appropriate scope (such as actions:read) and provide it via the github-token input [8][9]. The default GITHUB_TOKEN is generally scoped to the current run and does not have permission to access artifacts from other runs or repositories [8].
Citations:
- 1: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 3: https://docs.github.com/en/enterprise-cloud@latest/actions/reference/workflows-and-actions/workflow-syntax
- 4: https://github.github.com/gh-aw/reference/permissions/
- 5: https://help.github.com/en/enterprise-server@3.12/actions/security-for-github-actions/security-hardening-your-deployments/configuring-openid-connect-in-amazon-web-services
- 6: https://docs.github.com/en/enterprise-cloud@latest/actions/how-tos/secure-your-work/security-harden-deployments/oidc-in-cloud-providers
- 7: Wrong possible id-token permissions types listed github/docs#33483
- 8: https://github.com/actions/download-artifact
- 9: https://docs.github.com/en/actions/tutorials/store-and-share-data
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/RELEASING.md head/sections =="
if [ -f docs/RELEASING.md ]; then
sed -n '1,100p' docs/RELEASING.md
fi
echo "== all workflow and release action refs =="
rg -n "permissions:|id-token:|contents:|actions:|artifacts|pypa/gh-action-pypi-publish|release/v1|uses:" -S . --glob '!node_modules/**' --glob '!dist/**' --glob '!.git/**' --glob '*.yml' --glob '*.yaml' --glob '*.md' . 2>/dev/null | head -200 || true
echo "== list tracked files near docs and workflows =="
git ls-files docs workflows .github . 2>/dev/null | sed -n '1,120p'Repository: Deathcharge/agent-consensus
Length of output: 6858
🌐 Web query:
pygithub pythongithubpypi/gh-action-pypi-publish upload github action GITHUB_TOKEN download-artifact no contents read required
💡 Result:
The gh-action-pypi-publish GitHub Action, when used with PyPI's Trusted Publishing (the recommended approach), requires the id-token: write permission to exchange an OIDC token for authentication [1][2]. Regarding the GITHUB_TOKEN and potential requirements for contents: read: 1. Standard Trusted Publishing: You do not generally need contents: read if your publishing job only downloads artifacts (using actions/download-artifact) and uploads them to PyPI [3][4]. The publishing job should ideally be separate from your build job to minimize privilege exposure [1][5]. 2. Private Repositories / Repository Checkout: If your publishing job includes a step that checks out your repository (e.g., using actions/checkout), you will need the contents: read permission, especially for private repositories [3][6]. Without this permission, the checkout step will fail because the default GITHUB_TOKEN permissions are restricted when a custom permissions block is defined [6]. 3. Best Practice: To maintain security, it is strongly recommended to separate the build job from the publishing job [1][5]. The publishing job should only contain the download and upload steps and should be configured with the minimum required id-token: write permission [1][7]. If you find yourself needing contents: read in your publishing job, it is a strong indicator that you are likely performing build steps (like checking out code) in the wrong job [3].
Citations:
- 1: https://github.com/pypa/gh-action-pypi-publish?tab=readme-ov-file
- 2: https://docs.pypi.org/trusted-publishers/using-a-publisher/
- 3: Document permission requirements for private repositories pypa/gh-action-pypi-publish#237
- 4: https://packaging.python.org/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/
- 5: https://github.com/pypa/gh-action-pypi-publish
- 6: In Using a Publisher docs, call out that GitHub Actions permissions also need
contents: readto checkout private repositories pypi/warehouse#17148 - 7: https://github.com/pypi/warehouse/blob/main/docs/user/trusted-publishers/using-a-publisher.md
🌐 Web query:
gh-action-pypi-publish release/v1 commit SHA git ref current commit
💡 Result:
The release/v1 branch in the pypa/gh-action-pypi-publish repository is a mutable pointer [1][2]. When you use uses: pypa/gh-action-pypi-publish@release/v1 in your GitHub Actions workflow, the action will execute the latest commit present on that branch at the time the workflow runs [1][3]. Because release/v1 is a branch and not a static tag or specific commit hash, it is subject to updates by the repository maintainers [1][2]. To ensure security and reproducibility in your workflows, the official documentation strongly recommends pinning the action to an exact tagged version (e.g., @v1.14.0) or a full Git commit SHA (e.g., @cef2210) [1][4][3]. Using a commit SHA ensures that your workflow always runs the exact same code, protecting you from unexpected changes that might occur if the release/v1 branch is updated [1][2].
Citations:
- 1: https://github.com/pypa/gh-action-pypi-publish
- 2: https://github.com/marketplace/actions/pypi-publish
- 3: https://github.com/pypa/gh-action-pypi-publish?tab=readme-ov-file
- 4: https://github.com/pypa/gh-action-pypi-publish/releases
Make the release workflow guidance least-privilege and reproducible.
id-token: write is sufficient for the trusted-publishing upload, but artifact access is workflow- or artifact-flow-specific; this wording should require only the read scopes actually needed for the chosen artifact flow (no wider permissions if checkout isn’t required). Pin pypa/gh-action-pypi-publish to a reviewed full commit SHA in the release procedure rather than relying on a later step, since @release/v1 is a mutable branch ref.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/RELEASING.md` around lines 40 - 42, Update the release workflow guidance
to grant the publishing job only the permissions required by its selected
artifact flow, retaining id-token: write and avoiding checkout or broader read
scopes unless explicitly needed. Replace the mutable
pypa/gh-action-pypi-publish@release/v1 reference with a reviewed full commit SHA
directly in the documented release procedure.
| ## Decision semantics | ||
|
|
||
| ## 🎓 Learn More | ||
| - The default threshold is two thirds of **all configured participant weight**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate “two-thirds.”
Use “two-thirds of all configured participant weight” for grammatical correctness.
🧰 Tools
🪛 LanguageTool
[grammar] ~139-~139: Use a hyphen to join words.
Context: ...emantics - The default threshold is two thirds of **all configured participant w...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 139, Update the README sentence describing the default
threshold to hyphenate “two-thirds,” preserving the existing wording about all
configured participant weight.
Source: Linters/SAST tools
| def test_result_is_json_serializable_with_json_metadata() -> None: | ||
| result = evaluate_votes([Vote("a", "yes")]) | ||
|
|
||
| encoded = json.dumps(result.to_dict(), sort_keys=True) | ||
| assert '"status": "agreed"' in encoded | ||
| assert '"participant": "a"' in encoded |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the metadata serialization path.
This test creates no metadata, so it cannot catch regressions in the stated “with JSON metadata” contract. Add JSON-compatible metadata to the vote/response path and assert it survives to_dict() and json.dumps().
Proposed test adjustment
-def test_result_is_json_serializable_with_json_metadata() -> None:
- result = evaluate_votes([Vote("a", "yes")])
+def test_result_is_json_serializable_with_json_metadata() -> None:
+ result = evaluate_votes([Vote("a", "yes")])
encoded = json.dumps(result.to_dict(), sort_keys=True)
assert '"status": "agreed"' in encoded
assert '"participant": "a"' in encodedVote currently has no metadata field, so construct a ConsensusResult/ParticipantResponse fixture with JSON-compatible metadata, or rename this test to reflect what it actually verifies.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 190-190: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result.to_dict(), sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_consensus.py` around lines 188 - 193, Update
test_result_is_json_serializable_with_json_metadata to exercise the metadata
serialization path by constructing the vote/response or ConsensusResult fixture
with JSON-compatible metadata, then assert that metadata is preserved in both
to_dict() and json.dumps() output. If the current API cannot attach metadata,
rename the test to accurately describe the behavior it verifies.
1 participant
Summary
Verification
Owner gates
No PyPI publication, production deployment, or default-branch merge is performed by this PR.