Repository navigation
feat(release): bootstrap first-time winget submissions - #28
Conversation
Findings
Architecturally, the PR matches the plan: bootstrap is explicit and fail-closed, existing packages still use update mode, consumer repos are not special-cased, and codereview-cli can safely consume this after Verification note: I could not rerun the reported tests in this read-only session because Reference checked: https://github.com/microsoft/winget-create |
|
Findings
|
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 32d51e1
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 2 |
| harness-engineering:harness-enforcement-reviewer | 2 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 3 |
harness-engineering:harness-architecture-reviewer (2 findings)
actions/winget-submit/winget_submit.py:241
Both subprocess.run calls for wingetcreate (update path ~line 241, submit path ~line 256) have no timeout parameter. If wingetcreate hangs during GitHub API calls or PR creation against microsoft/winget-pkgs, the release job blocks until the runner job limit (~6 hours). WINGETCREATE_TIMEOUT_SECONDS is defined but only applied to the download step. Add timeout= to both subprocess.run calls and handle subprocess.TimeoutExpired.
💡 Suggestion - actions/winget-submit/action.yml:1
The composite action downloads and executes wingetcreate.exe, a Windows-only binary, but neither the description nor any runtime guard documents or enforces a Windows runner requirement. A non-Windows caller receives an opaque exec-format error rather than a clear failure. Add a platform assertion (e.g. checking $RUNNER_OS == 'Windows') and note the requirement in the action description.
harness-engineering:harness-enforcement-reviewer (2 findings)
💡 Suggestion - actions/winget-submit/winget_submit.py:225
load_release_assets and resolve_windows_assets are called before the package_exists check in run_submit. For the MODE_MISSING_BOOTSTRAP_DISABLED fast-fail path (bootstrap=false, package absent from winget-pkgs), these API round-trips are wasted. Moving package_exists before the asset-loading calls allows early exit without unnecessary I/O.
💡 Suggestion - actions/winget-submit/action.yml:51
The github-token input is mapped to the env var GITHUB_TOKEN, which conflicts with the GitHub Actions built-in default token. While there is no current bug since both values are the same in release.yml, a future caller passing a PAT would silently shadow the built-in token for all steps within this composite action. Use a distinct name such as WINGET_GITHUB_TOKEN to eliminate ambiguity.
harness-engineering:harness-knowledge-reviewer (1 findings)
.github/workflows/release.yml:352
References
open-cli-collective/.github/actions/winget-submit@v1, but this tag does not exist until it is explicitly advanced after this PR merges. A release triggered in the window between merge and tag-update will fail with 'uses: action not found'. Confirm the tag-advance process is in place before this workflow runs in production.
harness-engineering:harness-self-documenting-code-reviewer (3 findings)
actions/winget-submit/winget_submit.py:232
wingetcreate.exe is downloaded from aka.ms/wingetcreate/latest and executed with the winget submission token without any integrity verification (no hash check). A tampered CDN redirect or MITM could deliver an attacker-controlled binary that receives the token. Consider pinning to a versioned release URL and verifying a known SHA-256 before executing.
💡 Suggestion - actions/identity-check/identity.py:86
_winget_bootstrap() raises ManifestError for a non-boolean value while validate() appends the same condition as a collected error string. A caller invoking normalize() on an unvalidated manifest with bootstrap: 'yes' receives an unhandled exception instead of a structured error list. The duplicate paths can also silently diverge if one message is updated without the other. Either document that normalize() requires pre-validated input, or remove the raise from _winget_bootstrap and rely solely on the validate() path.
💡 Suggestion - actions/winget-submit/winget_submit.py:59
The parameter
package_exists: boolin select_mode shadows the module-level package_exists function. Any future modification that calls package_exists(...) inside select_mode will get TypeError: 'bool' object is not callable. Rename the parameter to pkg_exists or exists to remove the shadow.
3 info-level observations excluded. Run with --verbose to include.
2 PR discussion threads considered.
Completed in 6m 23s | $1.83 | 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 | 6m 23s wall · 18m 30s compute (Reviewers: 5m 16s · Synthesis: 1m 04s) |
| Cost | $1.83 |
| Tokens | 340.6k in / 71.5k out |
| Turns | 12 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 31.0k | 4.2k | 17.4k | 13.6k (1h) | $0.12 |
| database:database-reviewer | sonnet | 16.8k | 242 | 4.1k | 12.7k (1h) | $0.06 |
| harness-engineering:harness-architecture-reviewer | sonnet | 66.7k | 19.2k | 28.3k | 38.4k (1h) | $0.46 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 62.1k | 20.6k | 28.3k | 33.8k (1h) | $0.46 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 99.5k | 15.0k | 45.3k | 54.2k (1h) | $0.46 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 41.7k | 8.6k | 21.0k | 20.7k (1h) | $0.22 |
| security:security-code-auditor | haiku | 22.7k | 3.6k | 4.2k | 18.5k (1h) | $0.05 |
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
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 1907036 | Previous: 32d51e1 (incremental)
Summary
No issues found.
10 PR discussion threads considered.
Completed in 2m 49s | $1.36 | 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 49s wall · 4m 49s compute (Reviewers: 2m 01s · Synthesis: 15s) |
| Cost | $1.36 |
| Tokens | 340.0k in / 18.3k out |
| Turns | 7 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 39.2k | 801 | 17.4k | 21.8k (1h) | $0.10 |
| database:database-reviewer | sonnet | 24.8k | 46 | 4.1k | 20.6k (1h) | $0.08 |
| harness-engineering:harness-architecture-reviewer | sonnet | 59.7k | 7.1k | 4.1k | 55.6k (1h) | $0.32 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 61.1k | 5.6k | 4.1k | 57.0k (1h) | $0.30 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 55.5k | 88 | 4.1k | 51.3k (1h) | $0.20 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 33.3k | 104 | 4.1k | 29.1k (1h) | $0.11 |
| security:security-code-auditor | haiku | 40.6k | 3.1k | 4.2k | 36.4k (1h) | $0.06 |
| discussion-summarizer | — | 25.9k | 1.4k | 0 | 21.2k (1h) | $0.19 |
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.
## Summary `cr` ships no reviewers: with no profile `agent_sources`, no repository `.codereview/agents/` and no `--agents-dir`, `cr review` stops with "no reviewer agents available". Everything a review checks comes from agent prompts someone has to write. This adds `starter-agents/`, a general catalog to use as a profile agent source, and documents how to build on it. **Agents** (14): - Always run when their globs match (`required_on_match`): `general:correctness`, `general:repo-conventions` (checks changes against the rules a repository writes in `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING`, `ARCHITECTURE` and `docs/`, including nested `AGENTS.md` files above each changed path, and cites the document and section), and `documentation:docs`. - Selected per PR: `tests:behavior`, `security:trust-boundaries`, `architecture:design`, `structure:repo-health`, `automation:ci-release`, `go:implementation-tests`, `rust:implementation-tests`, `tauri:config-ipc`, `macos:swift-appkit`, `frontend:correctness`, `frontend:accessibility`. Every prompt shares one review bar: report only what the PR introduces or worsens, with a concrete consequence and the input or sequence that shows it, stated as confirmed or inferred, actionable from the changed code, and not already caught by a tool the repository runs. Each also has a "What not to report" list and a severity scale for its lane. **Docs:** `starter-agents/README.md` covers using the catalog as a profile source, extending it with an organization catalog and repository-local agents (including how a repository-local agent with the same ID replaces a starter agent), and turning `AGENTS.md` rules into a repository-local conventions reviewer. `README.md`, `docs/review-guidance.md` and `docs/development.md` point to it. **This repository:** the Rust, Tauri and macOS agents move from `.codereview/agents/` into the catalog, since they never apply to this repository's own PRs. The remaining repository agents replace `documentation:docs`, `structure:repo-health`, `automation:ci-release` and `go:implementation-tests` by ID when this repository is reviewed with the starter catalog as a profile source. **Test:** `internal/agents/starter_catalog_test.go` loads the catalog with the strict profile-source parser, requires the three baseline agents to stay `required_on_match`, and fails when `starter-agents/README.md` does not list an agent (checked by removing one row from the table). ## Validation Dry runs with the catalog passed through `--agents-dir`, pinned with `--review-base-sha`/`--review-head-sha` and `--without-discussion`, nothing posted: - open-cli-collective/atlassian-cli#478 at the first commit the existing reviewers saw (`ba163234`). The starter reviewers that ran (`general:correctness`, `documentation:docs`, `tests:behavior`, `architecture:design`, `go:implementation-tests`) reported 4 of the 6 problems the first published review raised: the lossy-write guard failing open when the storage body is missing, the undocumented `structured macros` rule, regex matching that only catches adjacent markup, and README text naming two constructs where the code refuses three. They also reported two the published review did not: the README adds `--allow-lossy` to the `page create` flag table though only `page edit` registers it, and the refusal tests never check that no write reached the server. `general:repo-conventions` reported nothing. - open-cli-collective/.github#28 at its final head: `general:correctness` reported that the checksum download sends an `Authorization` header that urllib carries across the asset API's redirect to the storage host (not yet confirmed against a live run), and `tests:behavior` reported untested fail-closed branches in the asset resolution. ## Type of Change - [ ] Bug fix - [x] New feature - [x] Documentation update - [ ] Repo scaffolding / maintenance ## Checklist - [x] I have read the contributing guide - [x] My change follows the project structure and conventions - [ ] `make tidy` passes - [x] `make lint` passes - [x] `make test` passes - [x] `make build` passes ## Related Issues Proposal for giving every reviewer base-pinned guidance without per-prompt discovery: #682. <!-- twapp-session: b7acef9e-0a9c-4af5-8ab8-775953bbc6e1 --> Model: Claude Opus 5.5 (1M context, xhigh effort) in Claude Code.
Summary
Tests
Closes #27