Document panic() contract for embed-guarded lazy-load panic sites - #54599
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Diff is pure comment additions documenting existing panic invariants; no logic, abstractions, or dead code to cut.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54599 does not have the implementation label and has only 25 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #54599 adds documentation (panic contract comments) to 7 production Go files without any behavioral test changes.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The added // Build-time invariant: comments clearly explain why each panic() is a programming-error guard rather than a user-facing failure path. The wording is consistent, accurate, and correctly distinguishes embedded-data corruption from dynamic user input. No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.6 AIC · ⌖ 8.94 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Pull request overview
Documents why internal panic paths are unreachable during normal operation without changing behavior.
Changes:
- Adds invariant comments for embedded JSON/cache failures.
- Documents type- and configuration-guarded marshal/registration failures.
Show a summary per file
| File | Description |
|---|---|
pkg/actionpins/data.go |
Documents embedded pin and cache invariants. |
pkg/workflow/agentic_engine.go |
Documents built-in registration panic. |
pkg/workflow/mcp_setup_gateway.go |
Documents string-slice marshaling invariant. |
pkg/workflow/mcp_setup_safe_outputs.go |
Documents fixed-config marshaling invariant. |
pkg/workflow/model_aliases.go |
Documents embedded alias loading failures. |
pkg/workflow/permissions_toolset_data.go |
Documents embedded toolset data failure. |
pkg/workflow/pi_engine.go |
Documents model payload marshaling failure. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| // Build-time invariant: payload above is built entirely from string/map | ||
| // literals, so Marshal cannot fail; json.Marshal only errors for | ||
| // non-serialisable types such as channels or functions. |
| // Build-time invariant: the built-in engines list above has fixed, non-empty | ||
| // names, so registration can only fail if a future refactor introduces a | ||
| // duplicate or empty name — a programming error caught by CI, not user input. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — one minor suggestion on comment self-containedness; otherwise LGTM.
📋 Key Themes & Highlights
Key Theme
- Cross-reference comment (
model_aliases.go:149): the publicBuiltinModelAliasesfunction's panic site usessee comment ... aboverather than repeating the invariant inline. Public API sites are read in isolation; better to be self-contained.
Positive Highlights
- ✅ Every panic site now carries a clear "build-time invariant" rationale
- ✅ PR correctly distinguishes embed-guarded panics from dynamic-input panics
- ✅ The
pi_engine.gocomment is a genuine improvement over the previous wording — it now names the non-serialisable types explicitly (channels, functions) - ✅ No behaviour change; the documentation is well-scoped and accurate
Skill selection used the fallback heuristic (documentation-only changed files → /grill-with-docs).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 23.4 AIC · ⌖ 9.92 AIC · ⊞ 7.8K
Comment /matt to run again
| @@ -145,6 +148,7 @@ func isBuiltinOnlyAliasMap(m map[string][]string) bool { | |||
| func BuiltinModelAliases() map[string][]string { | |||
| data, err := loadBuiltinModelAliases() | |||
There was a problem hiding this comment.
[/grill-with-docs] The cross-reference // Build-time invariant: see comment in getBuiltinOnlyAliasMap above. leaves BuiltinModelAliases (a public function) dependent on navigating to another function to understand the invariant. Readers encountering this in search results or generated docs won't have that context.
💡 Suggested inline wording
// Build-time invariant: model_aliases.json is embedded at compile time and
// validated by TestBuiltinModelAliases; unmarshal can only fail for corrupted
// release data, never dynamic user input.
panic(err)Mirrors the comment in getBuiltinOnlyAliasMap directly, making each site self-contained.
@copilot please address this.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — I don't see a behavioral regression in the code paths themselves, but this change does not add any executable guardrail for the new "build-time invariant" contract it documents.
Blocking theme
The patch explicitly reframes several panics as release-time invariants enforced by tests, yet it adds zero coverage tying those comments to the actual failure modes. That leaves the contract easy to drift: a future refactor can route non-embedded input through one of these helpers, keep the reassuring comment, and still panic at runtime. For a documentation-only hardening change whose whole premise is "CI guarantees this is unreachable," at least one focused test/assertion per pattern should exist to prove the guarantee instead of relying on prose.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.76 AIC · ⌖ 8.78 AIC · ⊞ 4.6K
Comment /review to run again
|
Please address the visible failed check and then run the Failed check from the compact queue: There is also still a blocking review from Run: https://github.com/github/gh-aw/actions/runs/32525213052
|
…x error message lint Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed both items.
Blocking review (executable guardrails) — added
Also corrected two inaccurate invariant comments from the earlier Copilot review: Validations: Human action needed: agent pushes don't trigger CI, so |
|
🎉 This pull request is included in a new release. Release: |
8
panic()calls inpkg/workflow/pkg/actionpinsfire only if lazily-loaded,go:embed-ed JSON data fails to (un)marshal — a condition that should be build-time-impossible. These relied only on thepanic-in-library-codelinter's incidental exemption (e.g.BUG:prefix,sync.Once) rather than an explicit documented contract, risking a future refactor accidentally routing dynamic input through one of these paths.pkg/workflow/model_aliases.go(2 sites): embeddedmodel_aliases.jsonunmarshalpkg/actionpins/data.go(3 sites): cache init, embeddedaction_pins.jsonunmarshal, empty-SHA validationpkg/workflow/agentic_engine.go: built-in engine registrationpkg/workflow/permissions_toolset_data.go: embedded GitHub toolsets JSON unmarshalpkg/workflow/pi_engine.go: internal payload marshalpkg/workflow/mcp_setup_gateway.go: internal env var name list marshalpkg/workflow/mcp_setup_safe_outputs.go: internal file render config marshalTestBuiltinModelAliases, actionpins internal tests,TestToolsetPermissionsLoadedFromJSON), so corrupted data failsgo testrather than only surfacing as a runtime panic — no new tests needed.