yama: add soundness/polarity lens + 2 blocking criteria (review follow-up to #56)#58
Conversation
…w-up to #56) Applies the review feedback from PR #56 that merged before it could be folded in. The Yama reviewer was steered toward 'no Obj.magic/@unwrap/%identity' but not toward the subtler class of bug that every mechanical gate (golden compile, bench) passes green on — an unsound type that compiles. That's exactly the #55 regression a human had to catch (an output-only 'a from an `any` getter return). - workflowInstructions: a 'compilation is NOT soundness' lens (CI proves compiles + matches baseline, not soundness; a fake behind a shared typeRef doesn't bubble to the report) and a PRODUCE/RECEIVE polarity rule ('a is sound only in a consumer-supplied input position). - focusAreas: an all-cases-or-flag line (every union/overload variant must be reachable or the whole prop is flagged — never silently dropped). - blockingCriteria (+2 -> 6): output-only 'a that doesn't round-trip; a fake emitted UNFLAGGED inside a shared *Types.res entry. Config-only; doesn't touch the generator.
Benchmark: ✅ PASS
|
commit: |
…ew note) Per review on #58: the 'fake string inside a shared *Types.res entry' blocking criterion is judgment-heavy — an LLM could flag legitimately-string fields. Add an explicit guard: only BLOCK after VERIFYING against the source .d.ts that the TS type was genuinely richer and got narrowed; a field that is legitimately `string` in TypeScript is not a violation.
|
Thanks for the thorough review — and for running the YAML parse I couldn't (good to have psych confirm the backticks/colons/apostrophes in the quoted strings are clean). Applied your one actionable note: tightened the hidden-fake criterion to curb false positives. It now requires the reviewer to VERIFY against the original On your other two notes — both intentional, leaving as-is:
Pushed as ef903fd. |
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
File reviewed: yama.config.yaml (1 file, +27 lines)
New issues raised: 0
Assessment
This PR successfully adds critical soundness guidance to the Yama review configuration based on lessons from issues #55, #40, and #49. The changes are:
- CRITICAL LENS — Compilation is NOT soundness; CI gates prove compilation + baseline match, not type safety
- POLARITY —
'ais only sound in consumer-supplied INPUT positions (round-trips); OUTPUT-only'aunifies with anything - All-cases-or-flag — Every union/overload variant must be reachable; never silently drop cases
- Two new blocking criteria — Output-only
'aand unflagged fakes in shared types now block merges
Verification:
- ✅ YAML structure valid with consistent 2-space/4-space indentation
- ✅ Blocking criteria count: 4 → 6 (matches PR description)
- ✅ All three historical issues (#55, #40, #49) addressed in guidance
- ✅ Config-only change — no generator code modified
Decision: Approve. The tightened criteria will help catch unsound types that compile successfully, addressing the highest-value blind spot in automated review.
What
Follow-up to #56 (merged before this feedback could be folded in). Applies the three review suggestions to
yama.config.yamlso the AI reviewer is pointed at the one blind spot it currently has — an unsound type that compiles.Why
The config (correctly) steers Yama away from re-running the mechanical CI gates, and blocks
Obj.magic/@unwrap/%identity. But the highest-value bugs in our review history were none of those — they compiled green on every gate and needed a human:anyfield on a getter return became an output-only'a(routes(): RouterRoute<'a>[]) that unifies with anything. bench was 8/8.=> stringreturn hidden inside a shared@unboxed, rendered unflagged.As configured, the reviewer would likely miss these too. This PR closes that.
Changes (config-only — does not touch the generator)
workflowInstructions— a "compilation is NOT soundness" lens (CI proves compiles + matches baseline, not soundness; a fake behind a shared typeRef doesn't bubble to the report) + a PRODUCE/RECEIVE polarity rule ('ais sound only in a consumer-supplied input).focusAreas(mapping correctness) — an all-cases-or-flag line: every union/overload variant must be reachable, or the whole prop is flagged — never silently dropped.blockingCriteria(4 → 6) — (a) an output-only'athat doesn't round-trip; (b) a fake emitted UNFLAGGED inside a shared*Types.resentry.Verified: all five additions present, blocking criteria count 4 → 6, indentation mirrors the existing entries. (No YAML linter available locally; structure matches the surrounding blocks exactly.)