Skip to content

fix(winget): preserve bootstrap schema headers - #30

Merged
rianjs merged 3 commits into
mainfrom
fix/29-winget-schema-headers
Jun 4, 2026
Merged

fix(winget): preserve bootstrap schema headers#30
rianjs merged 3 commits into
mainfrom
fix/29-winget-schema-headers

Conversation

@rianjs

@rianjs rianjs commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • prepend winget schema headers when rendering first-submission bootstrap manifests
  • fail fast when ManifestType or ManifestVersion is missing
  • add regression coverage for exact first-line schema headers

Closes #29

Verification

  • 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

@rianjs

rianjs commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Findings

No findings. The PR is tightly scoped to the shared winget-submit bootstrap renderer and directly addresses the v0.3.58 failure mode: rendered split manifests now get an exact first-line winget schema header, and missing ManifestType / ManifestVersion fails before wingetcreate submit.

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.

@rianjs

rianjs commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author
  • Major /Users/rianjs/dev/.github/actions/winget-submit/test_winget_submit.py:249 The negative case only removes ManifestVersion from the installer manifest. Because render_bootstrap_manifests() renders the version and locale files before it reaches the installer, this test still passes if the function writes partial output and only fails later. That does not prove the “fail fast” part of the ticket. I’d assert the output dir stays empty on error, or make the missing metadata hit the first manifest.

  • Minor /Users/rianjs/dev/.github/actions/winget-submit/test_winget_submit.py:224 The new coverage is still narrow enough that a broken ManifestType path or a hardcoded schema version could slip through. The happy-path checks only use 1.10.0, so an implementation that always emits that version would still satisfy the test. A tiny parameterized failure test over both required keys, plus one alternate schema-version fixture, would close that gap.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

⚠️ Should Fix - 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.

Comment thread actions/winget-submit/test_winget_submit.py
Comment thread actions/winget-submit/winget_submit.py Outdated
Comment thread actions/winget-submit/winget_submit.py
@monit-reviewer
monit-reviewer dismissed their stale review June 4, 2026 14:15

Superseded by updated review

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rianjs
rianjs merged commit 1bf8a15 into main Jun 4, 2026
19 checks passed
@rianjs
rianjs deleted the fix/29-winget-schema-headers branch June 4, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve winget schema headers during bootstrap render

2 participants