fix(winget): preserve bootstrap schema headers - #30
Conversation
FindingsNo findings. The PR is tightly scoped to the shared The change does not drift into package naming, bootstrap selection, token handling, update behavior, or consumer templates. Regression coverage also checks the raw rendered text rather than only reparsed YAML, which is the right guard for this bug. |
|
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 2c52cd6
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-enforcement-reviewer | 1 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-enforcement-reviewer (1 findings)
actions/winget-submit/test_winget_submit.py:249
The negative test removes a required key only from the installer manifest, which is the last item in the render loop ([version_manifest, *locale_manifests, installer_manifest]). While the two-phase design prevents partial writes regardless, the test never exercises a failure on the first or second manifest — a corrupted version or locale template would not be caught. This means the test does not fully prove the 'fail fast before any output' guarantee stated in the PR. Add at least one parametrized case that removes the key from the version manifest (processed first), or strengthen the assertion to confirm the output directory is empty on error. The PR author flagged this gap as Major in the self-review thread and it remains open.
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - actions/winget-submit/winget_submit.py:177
output.mkdir(parents=True, exist_ok=True) is called unconditionally before the validation/collection loop. If _dump_manifest() raises SubmitError during collection, the output directory is left on disk as an empty artifact. Callers or downstream steps that check for directory existence as a proxy for success will get a false positive. Moving mkdir to just before the write loop (after collection succeeds) would make on-disk state unambiguous on failure, consistent with the 'no partial output' contract.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - actions/winget-submit/winget_submit.py:344
_required_manifest_string() uses value.strip() to check for emptiness but returns the original value. A string with leading or trailing whitespace passes the guard and is embedded verbatim in the schema URL, producing a malformed URL. Additionally, ManifestType is not validated against the known set {version, installer, defaultLocale, locale}, so a typo silently produces an incorrect but syntactically valid schema URL. Return value.strip() instead of value, and consider validating ManifestType against the known set.
2 info-level observations excluded. Run with --verbose to include.
2 PR discussion threads considered.
Completed in 3m 45s | $0.89 | 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 | 3m 45s wall · 9m 48s compute (Reviewers: 2m 34s · Synthesis: 1m 09s) |
| Cost | $0.89 |
| Tokens | 230.6k in / 38.1k out |
| Turns | 14 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 30.6k | 5.0k | 17.4k | 13.2k (1h) | $0.13 |
| database:database-reviewer | sonnet | 26.6k | 3.0k | 16.2k | 10.4k (1h) | $0.09 |
| harness-engineering:harness-architecture-reviewer | sonnet | 34.3k | 6.9k | 18.1k | 16.2k (1h) | $0.17 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 47.5k | 3.4k | 34.8k | 12.7k (1h) | $0.11 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 36.6k | 9.1k | 18.1k | 18.4k (1h) | $0.22 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 41.9k | 3.5k | 31.1k | 10.8k (1h) | $0.10 |
| security:security-code-auditor | haiku | 13.2k | 7.3k | 0 | 13.2k (1h) | $0.06 |
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: cb175d8 | Previous: 2c52cd6 (incremental)
Summary
No issues found.
5 PR discussion threads considered.
Completed in 1m 47s | $0.66 | 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 | 1m 47s wall · 2m 58s compute (Reviewers: 60s · Synthesis: 29s) |
| Cost | $0.66 |
| Tokens | 201.4k in / 14.0k out |
| Turns | 7 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 38.5k | 1.5k | 17.4k | 21.1k (1h) | $0.11 |
| database:database-reviewer | sonnet | 17.4k | 255 | 4.1k | 13.3k (1h) | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 26.0k | 98 | 4.1k | 21.9k (1h) | $0.08 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 22.6k | 106 | 4.1k | 18.5k (1h) | $0.07 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 27.1k | 83 | 2.9k | 24.1k (1h) | $0.09 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 18.0k | 3.1k | 4.1k | 13.8k (1h) | $0.10 |
| security:security-code-auditor | haiku | 27.0k | 8.1k | 4.2k | 22.9k (1h) | $0.07 |
| discussion-summarizer | — | 24.8k | 726 | 16.2k | 3.8k (1h) | $0.08 |
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
ManifestTypeorManifestVersionis missingCloses #29
Verification
uv run --with PyYAML==6.0.2 --with pytest>=8,<9 python -m pytest -qinactions/winget-submituv run --with PyYAML==6.0.2 --with pytest>=8,<9 python -m pytest -qinactions/release-preflightuv run --with PyYAML==6.0.2 --with pytest>=8,<9 --with defusedxml==0.7.1 python -m pytest -qinactions/identity-check