fix(chocolatey): handle first-submission moderation by rianjs · Pull Request #32 · open-cli-collective/.github

rianjs · 2026-06-04T14:32:31Z

Summary

add a chocolatey-push action for Chocolatey pack/push behavior
treat only the documented first-submission moderation 403 state as a green pending state
keep ordinary push failures, visible package conflicts, bad keys, and ambiguous Chocolatey probes hard-failing
wire the reusable release workflow to use the new action and add CI coverage

Closes #31

Verification

uv run --with pytest>=8,<9 python -m pytest -q in actions/chocolatey-push
uv run --with PyYAML==6.0.2 --with pytest>=8,<9 python -m pytest -q in actions/winget-submit
uv run --with PyYAML==6.0.2 --with pytest>=8,<9 python -m pytest -q in actions/release-preflight
uv run --with PyYAML==6.0.2 --with pytest>=8,<9 --with defusedxml==0.7.1 python -m pytest -q in actions/identity-check
actionlint

Closes #31

rianjs · 2026-06-04T14:34:17Z

Findings

Major: actions/chocolatey-push/chocolatey_push.py:95 treats any 403 (Forbidden) as suppressible once the package probes show “direct package exists + OData empty.” That can mask a bad API key during the same first-submission moderation window: Chocolatey documents invalid API keys as another 403 cause, and the probes would still report the pending package state from the earlier successful push. This conflicts with the PR intent to keep bad keys hard-failing. Please add a 403 invalid-key/unauthorized-output test and fail before applying the moderation exception when the push output indicates credential or ownership failure.

Reference: Chocolatey common 403 causes include invalid key, ownership conflicts, and moderation state: https://docs.chocolatey.org/en-us/community-repository/maintainers/common-errors/

rianjs · 2026-06-04T14:35:48Z

Findings

No findings. The updated diff addresses the prior major: credential/ownership-looking 403 output now hard-fails before Chocolatey moderation probes, and the regression test verifies no probe is attempted for an invalid-key-looking response.

The moderation exception remains narrow: only generic 403 Forbidden plus clean “package exists + no approved/listed entries” probes is treated as green pending state, with a warning summary. Ordinary failures, visible packages, missing packages, bad probes, and credential/owner signals still fail the release.

rianjs · 2026-06-04T14:37:39Z

The branch matrix is mostly there, but I would not call this fully proven yet.

Major: test_forbidden_invalid_key_output_fails_before_probe only exercises one credential-shaped 403 string. _looks_like_credential_or_owner_failure() also treats unauthorized, not owned, not the owner, and package owner as credential/ownership failures, but none of those variants are covered. A regression that mishandles the ownership-style 403s would still ship.
Major: test_pack_and_push_success and the moderation tests only assert the command names, not the exact choco push argv or cwd. They would still pass if the action stopped using --source https://push.chocolatey.org/, passed the wrong key, or ran from the wrong directory. Since this PR swaps the release workflow over to the new composite action, that leaves the most important wiring unverified.
Minor: Probe-fail-closed coverage in test_forbidden_probe_uncertainty_fails_closed stops at 503/malformed XML/Atom feed. It does not exercise transport-layer failures from request_text() such as URLError or timeout, so that fail-closed path is still untested.

monit-reviewer

Automated PR Review

Reviewed commit: e837cbd

Summary

Reviewer	Findings
database:database-reviewer	1
harness-engineering:harness-enforcement-reviewer	2
harness-engineering:harness-self-documenting-code-reviewer	2

database:database-reviewer (1 findings)

💡 Suggestion - actions/chocolatey-push/chocolatey_push.py:122

OData filter is constructed by direct interpolation of package_id without escaping: f"Id eq '{package_id}'". A package ID containing a single quote produces a malformed query, causing probe_package_state to receive an unexpected response and raising a misleading ProbeError. In practice Chocolatey IDs are alphanumeric-with-hyphens, but the fix is trivial: escape embedded single quotes by doubling them (package_id.replace("'", "''")).

harness-engineering:harness-enforcement-reviewer (2 findings)

⚠️ Should Fix - actions/chocolatey-push/action.yml:24

Direct ${{ inputs.package-id }} and ${{ inputs.working-directory }} interpolation inside a bash run: step is a known GitHub Actions shell-injection vector. Values containing shell metacharacters or $() substitution syntax will be evaluated by the shell. Map both inputs to env vars (e.g., PACKAGE_ID: ${{ inputs.package-id }}) and reference them as "$PACKAGE_ID" in the script — matching the existing secure pattern already used for CHOCO_API_KEY.

💡 Suggestion - actions/chocolatey-push/chocolatey_push.py:7

Uses xml.etree.ElementTree to parse the external Chocolatey API response instead of defusedxml. Every other action in this repo that parses external XML (identity-check, winget-submit, release-preflight) takes a defusedxml==0.7.1 dependency; chocolatey-push is the only exception. While CPython's expat backend is not vulnerable to XXE, defusedxml also guards against XML-bomb DoS. Replace the import and add defusedxml to the pytest install line in test-actions.yml to align with the repo's established XML-parsing policy.

harness-engineering:harness-self-documenting-code-reviewer (2 findings)

⚠️ Should Fix - actions/chocolatey-push/test_chocolatey_push.py:116

The parametrized matrix for the credential/owner 403 guard covers only a subset of patterns in _looks_like_credential_or_owner_failure. Missing coverage includes "invalid apikey", "api key is invalid", "not authorized", "not owned", "not the owner", and "package owner". A regression on any uncovered pattern would allow a credential or ownership 403 to bypass the hard-fail path and be misclassified as a green first-submission moderation state. This gap is flagged as an open concern in the PR discussion.

⚠️ Should Fix - actions/chocolatey-push/chocolatey_push.py:79

pack_and_push tests only assert command names, not the exact choco push argv (source URL, --key value, resolved .nupkg filename) or cwd. Tests would pass if the action used the wrong API key, pointed at the wrong source registry, or ran in the wrong working directory. Tighten assertions to cover the full command list and cwd, mirroring the pattern used in winget-submit. Raised as an open concern in the PR discussion.

5 info-level observations excluded. Run with --verbose to include.

3 PR discussion threads considered.

Completed in 7m 59s | $1.80 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, database:database-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	7m 59s wall · 18m 54s compute (Reviewers: 5m 39s · Synthesis: 2m 18s)
Cost	$1.80
Tokens	302.5k in / 72.4k out
Turns	15

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	32.0k	9.0k	17.4k	14.6k (1h)	$0.20
database:database-reviewer	sonnet	28.3k	1.6k	7.1k	21.2k (1h)	$0.11
harness-engineering:harness-architecture-reviewer	sonnet	52.4k	17.9k	7.1k	45.4k (1h)	$0.45
harness-engineering:harness-enforcement-reviewer	sonnet	41.4k	6.7k	7.1k	34.3k (1h)	$0.24
harness-engineering:harness-knowledge-reviewer	sonnet	55.1k	20.2k	7.1k	48.0k (1h)	$0.49
harness-engineering:harness-self-documenting-code-reviewer	sonnet	76.8k	8.9k	54.6k	22.2k (1h)	$0.24
security:security-code-auditor	haiku	16.4k	8.1k	0	16.4k (1h)	$0.07

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

Superseded by updated review

monit-reviewer

Automated PR Review

Reviewed commit: 956cee5 | Previous: e837cbd (incremental)

Approved with 1 non-blocking suggestion below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-self-documenting-code-reviewer	1

harness-engineering:harness-self-documenting-code-reviewer (1 findings)

💡 Suggestion - actions/chocolatey-push/chocolatey_push.py:158

_parse_atom_entries only catches ET.ParseError, but defusedxml raises a separate exception hierarchy (DefusedXmlException and subclasses like DTDForbidden, EntitiesForbidden) that does NOT inherit from xml.etree.ElementTree.ParseError. If the Chocolatey API returns XML with a DTD or entity references, those exceptions escape uncaught, bypass the except (PushError, ProbeError) handler in main, and surface as a raw traceback. The release still fails correctly, but the error output is unhelpful. Broaden the catch to include the defusedxml base exception class and re-raise as ProbeError.

8 PR discussion threads considered.

Completed in 2m 33s | $0.99 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 2 · Session resumed
Reviewers	hybrid-synthesis, database:database-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 33s wall · 3m 10s compute (Reviewers: 1m 52s · Synthesis: 15s)
Cost	$0.99
Tokens	304.4k in / 12.1k out
Turns	7

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	45.2k	906	17.4k	27.8k (1h)	$0.12
database:database-reviewer	sonnet	20.0k	67	4.1k	15.9k (1h)	$0.06
harness-engineering:harness-architecture-reviewer	sonnet	45.4k	92	4.1k	41.2k (1h)	$0.16
harness-engineering:harness-enforcement-reviewer	sonnet	34.1k	89	4.1k	30.0k (1h)	$0.12
harness-engineering:harness-knowledge-reviewer	sonnet	47.7k	117	4.1k	43.5k (1h)	$0.17
harness-engineering:harness-self-documenting-code-reviewer	sonnet	27.4k	7.1k	4.1k	23.2k (1h)	$0.19
security:security-code-auditor	haiku	33.7k	2.9k	4.2k	29.6k (1h)	$0.05
discussion-summarizer	—	50.9k	820	36.9k	9.3k (1h)	$0.12

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-06-04T14:52:31Z

+def _parse_atom_entries(body: str) -> int:
+    try:
+        root = ET.fromstring(body)
+    except ET.ParseError as exc:


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): _parse_atom_entries only catches ET.ParseError, but defusedxml raises a separate exception hierarchy (DefusedXmlException and subclasses like DTDForbidden, EntitiesForbidden) that does NOT inherit from xml.etree.ElementTree.ParseError. If the Chocolatey API returns XML with a DTD or entity references, those exceptions escape uncaught, bypass the except (PushError, ProbeError) handler in main, and surface as a raw traceback. The release still fails correctly, but the error output is unhelpful. Broaden the catch to include the defusedxml base exception class and re-raise as ProbeError.

Reply to this thread when addressed.

fix(chocolatey): handle first-submission moderation

888cbf3

Closes #31

fix(chocolatey): fail bad credentials before moderation skip

1cff6c9

test(chocolatey): cover push wiring and probe failures

e837cbd

monit-reviewer previously requested changes Jun 4, 2026

View reviewed changes

Comment thread actions/chocolatey-push/action.yml

Comment thread actions/chocolatey-push/test_chocolatey_push.py

Comment thread actions/chocolatey-push/chocolatey_push.py

Comment thread actions/chocolatey-push/chocolatey_push.py

Comment thread actions/chocolatey-push/chocolatey_push.py

fix(chocolatey): tighten moderation push guard

956cee5

monit-reviewer approved these changes Jun 4, 2026

View reviewed changes

rianjs merged commit c28cecb into main Jun 4, 2026
20 checks passed

rianjs deleted the fix/31-chocolatey-moderation branch June 4, 2026 14:53

Navigation Menu

Uh oh!

fix(chocolatey): handle first-submission moderation - #32

fix(chocolatey): handle first-submission moderation#32
rianjs merged 4 commits into
mainfrom
fix/31-chocolatey-moderation

rianjs commented Jun 4, 2026

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rianjs commented Jun 4, 2026

Summary

Verification

Uh oh!

rianjs commented Jun 4, 2026

Findings

Uh oh!

rianjs commented Jun 4, 2026

Findings

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants