Skip to content

Tutorial: Review Process

This tutorial teaches you to navigate Mistaber's encoding review process successfully. You will learn what reviewers look for, common rejection reasons, how to address feedback effectively, and how to get your encoding merged on the first attempt.

Duration: 1 hour Difficulty: Advanced Prerequisites: Tutorial 04: Testing Strategies

The Review Pipeline

Every encoding passes through a structured review pipeline:

graph TD
    A[PR Submitted] --> B[Automated Checks]

    B -->|Pass| C[Technical Review]
    B -->|Fail| D[Return for Fixes]

    C -->|Pass| E[Halachic Review]
    C -->|Fail| D

    E -->|Pass| F[Final Approval]
    E -->|Fail| D
    E -->|Questions| G[Discussion]

    G --> E

    F --> H[Merge]
    D --> I[Author Revises]
    I --> A

    style B fill:#e3f2fd
    style C fill:#fff9c4
    style E fill:#fce4ec
    style F fill:#c8e6c9

Automated Checks

Before human review, your PR must pass automated checks.

What Automated Checks Verify

Check What It Tests Common Failures
CI Build Code compiles Syntax errors, missing includes
Type Checker Predicates valid Unknown predicate, wrong arity
Test Suite All tests pass Assertion failures, UNSAT
Lint Code style Formatting issues
Coverage Threshold met Below 80% coverage

How to Pre-Check Locally

Before pushing, run locally:

# Run full test suite
pytest tests/ --cov=mistaber

# Check type system
mistaber typecheck

# Verify compilation
mistaber compile --check

# Run lint
ruff check .

Interpreting CI Failures

Common CI Error Messages 
ERROR: Unknown predicate 'custom_pred/2' in mechaber.lp:45
FIX: Add predicate to registry or use existing predicate

ERROR: Sort mismatch: expected 'food', got 'mixture' in line 67
FIX: Verify predicate arguments match expected sorts

ERROR: Test test_beheima_chalav failed - UNSAT
FIX: Rules contradict; check for conflicting assertions

ERROR: Coverage 75% below threshold 80%
FIX: Add more test cases

Technical Review

Technical reviewers focus on code quality and correctness.

What Technical Reviewers Check

Category Items
Naming Semantic rule IDs (r_topic_action), no siman:seif naming
Metadata Every rule has makor/2, madrega/2, scope/2
Override Descriptive values (never 'invalid')
Structure Correct file locations, includes work
Patterns Follows existing codebase patterns
Testing Meets minimum counts, covers edge cases

Technical Review Checklist

Reviewers use this checklist:

## Technical Review Checklist

### Naming and Structure
- [ ] Rule IDs are semantic with r_ prefix
- [ ] No siman:seif based naming
- [ ] Files in correct directory
- [ ] Follows existing patterns

### Metadata
- [ ] All rules have rule/1 declaration
- [ ] All normative rules have makor/2
- [ ] All normative rules have madrega/2
- [ ] All rules have scope/2

### Override Mechanism
- [ ] Override uses descriptive values
- [ ] No 'invalid', 'false', or 'null' values
- [ ] Positive assertions accompany overrides

### Testing
- [ ] 5+ positive tests
- [ ] 3+ negative tests
- [ ] 2+ edge cases
- [ ] Multi-world tests (if applicable)
- [ ] All tests pass

### Code Quality
- [ ] File header comments present
- [ ] Section comments for groups
- [ ] Inline comments for complexity
- [ ] No type checker errors/warnings

Addressing Technical Feedback

Common technical review comments and how to address them:

Comment: 'Rule name should be semantic' 
REVIEWER: "Rule name r_87_3 should be semantic"

WRONG:
rule(r_87_3).

CORRECT:
rule(r_bb_dag_sakana).  % Describes what the rule is about

RESPONSE:
"Fixed. Renamed rule to r_bb_dag_sakana to describe the
fish/dairy sakana content."
Comment: 'Missing makor citation' 
REVIEWER: "Normative rule r_bb_chaya_achiila lacks makor"

WRONG:
rule(r_bb_chaya_achiila).
madrega(r_bb_chaya_achiila, d_rabanan).
asserts(mechaber, issur(...)).

CORRECT:
rule(r_bb_chaya_achiila).
makor(r_bb_chaya_achiila, sa("yd:87:3")).  % Added!
madrega(r_bb_chaya_achiila, d_rabanan).
asserts(mechaber, issur(...)).

RESPONSE:
"Fixed. Added makor citation sa('yd:87:3')."
Comment: 'Override uses invalid' 
REVIEWER: "override/3 must use descriptive value, not 'invalid'"

WRONG:
override(rema, sakana(M), invalid) :-
    is_dag_chalav_mixture(M).

CORRECT:
override(rema, sakana(M), no_sakana) :-
    is_dag_chalav_mixture(M).

RESPONSE:
"Fixed. Changed override value from 'invalid' to descriptive
'no_sakana' per Mistaber conventions."

Halachic Review

Halachic reviewers verify accuracy against source texts.

What Halachic Reviewers Check

Category Items
Source Fidelity Encoding matches SA text accurately
Madrega Correct d_oraita/d_rabanan classification
Machloket Both positions accurately represented
Scope Correct world assignment
Implications Correct halachic outcomes

Halachic Review Checklist

## Halachic Review Checklist

### Source Verification
- [ ] Encoding matches SA text
- [ ] Rema gloss checked (agreement or machloket)
- [ ] Madrega verified from source
- [ ] Makor chain reaches authoritative source

### Accuracy
- [ ] Rules produce correct halachic outcomes
- [ ] Machloket correctly represented
- [ ] Override direction correct
- [ ] No missing conditions from source

### Practical Implications
- [ ] Query results match expected psak
- [ ] Edge cases handled correctly
- [ ] No unintended leniencies or stringencies

Addressing Halachic Feedback

Comment: 'Madrega should be d_rabanan' 
REVIEWER: "SA YD 87:3 explicitly states of+chalav is d_rabanan,
but encoding shows d_oraita"

WRONG:
madrega(r_bb_of_achiila, d_oraita).

CORRECT:
madrega(r_bb_of_achiila, d_rabanan).

RESPONSE:
"You're correct. SA 87:3 states 'eino assur ela miderabanan'.
Fixed madrega to d_rabanan."
Comment: 'Missing Rema position' 
REVIEWER: "Rema glosses on this seif but encoding only has Mechaber"

ANALYSIS:
1. Re-read Rema's gloss
2. Determine if it's disagreement or addition
3. If disagreement: add override in rema world
4. If addition: add separate rule with rema scope

RESPONSE:
"Added Rema's position. The Rema adds a stringency not in
Mechaber. Encoded as separate rule with scope(r_rema_addition, rema)."
Comment: 'Query produces incorrect outcome' 
REVIEWER: "Query for fish+dairy in ashk_mb should return heter,
but returns nothing"

ANALYSIS:
1. Check Rema's override propagates to ashk_mb
2. Verify ashk_mb inherits from rema
3. Check override syntax

FIX:
override(rema, sakana(M), no_sakana) :-
    is_dag_chalav_mixture(M).
asserts(rema, heter(achiila, M)) :-  % Was missing!
    is_dag_chalav_mixture(M).

RESPONSE:
"Fixed. The override blocked sakana but didn't assert heter.
Added explicit heter assertion that now propagates to ashk_mb."

Common Rejection Reasons

Category 1: Metadata Issues

Reason Frequency Prevention
Missing makor High Use encoding template
Wrong madrega Medium Verify from source text
Missing scope Medium Always declare scope
Invalid override value High Use descriptive values

Category 2: Accuracy Issues

Reason Frequency Prevention
Wrong world Medium Check who stated the ruling
Missing machloket position Medium Check Rema gloss
Incorrect inheritance Low Add multi-world tests
Missing condition Medium Read source carefully

Category 3: Testing Issues

Reason Frequency Prevention
Insufficient tests High Meet minimums before PR
Missing multi-world tests Medium Test inheritance
Failing tests Low Run locally first
No edge cases Medium Consider boundaries

The Revision Workflow

When you receive feedback requiring changes:

Step 1: Read All Comments

Before making any changes: 1. Read every comment in full 2. Understand what's being requested 3. Ask for clarification if unclear

Step 2: Acknowledge and Plan

## Revision Plan

Thank you for the review. I understand the following changes are needed:

1. **Makor citation**: Add makor for r_bb_chaya_achiila
2. **Override value**: Change 'invalid' to 'no_sakana'
3. **Multi-world test**: Add test for ashk_mb inheritance

I'll address all items and request re-review.

Step 3: Make Changes

Address each comment systematically:

# For each comment:
# 1. Make the fix
# 2. Add/update tests if needed
# 3. Verify locally
# 4. Mark as done in your tracking

Step 4: Document Responses

Use this format for each comment:

### Comment 1: Missing makor
**Status**: Fixed
**Action**: Added `makor(r_bb_chaya_achiila, sa("yd:87:3"))`
**Verification**: Type checker passes

### Comment 2: Override value
**Status**: Fixed
**Action**: Changed `invalid` to `no_sakana`
**Verification**: Test test_rema_no_sakana now passes

### Comment 3: Multi-world test
**Status**: Fixed
**Action**: Added test_ashk_mb_inherits_heter()
**Verification**: All 15 tests pass

Step 5: Request Re-Review

Only request re-review when: - [ ] All comments addressed - [ ] All tests pass locally - [ ] No new issues introduced - [ ] CI is green

@reviewer Ready for re-review. All 3 comments addressed:
- Added missing makor
- Fixed override value
- Added multi-world test

All tests pass. Please review when you have time.

Getting Expert Halachic Review

Some encodings require expert halachic review beyond standard technical review.

When to Request Expert Review

Scenario Reason
D'oraita content Higher stakes require verification
Complex machloket Multiple authorities, nuanced positions
Novel interpretation Not directly stated in source
Medical/scientific halacha Sakana, refuah considerations
Minhag variations Community-specific practices

How to Request Expert Review

## Request for Expert Halachic Review

**Encoding**: YD 87:3 (fish and dairy machloket)

**Reason for request**: This machloket involves:
1. Sakana (health danger) classification
2. Taz's scribal error argument
3. Shach's conditional interpretation
4. Different community practices

**Specific questions**:
1. Is encoding Mechaber's position as sakana (not issur) correct?
2. Should Taz's interpretation be encoded as override or annotation?
3. How to encode Shach's "only if cooked together" condition?

**Sources reviewed**:
- SA YD 87:3
- Beit Yosef YD 87
- Taz YD 87:3
- Shach YD 87:5

I believe the encoding is correct but would appreciate expert verification
given the sensitivity of this machloket.

Final Approval Criteria

Your PR is ready for merge when:

All Automated Checks Pass

  • [ ] CI build succeeds
  • [ ] Type checker passes with no errors/warnings
  • [ ] All tests pass
  • [ ] Coverage threshold met

Technical Review Approved

  • [ ] At least 1 technical approval
  • [ ] No unresolved technical comments
  • [ ] Code follows patterns and conventions

Halachic Review Approved (if required)

  • [ ] Halachic accuracy verified
  • [ ] Source citations confirmed
  • [ ] Machloket correctly represented

General Requirements

  • [ ] No merge conflicts
  • [ ] Branch is up to date with main
  • [ ] All discussions resolved

Tips for First-Attempt Approval

Before Submitting

  1. Use the encoding template: Ensures all metadata present
  2. Run the validate skill: Catches common issues
  3. Complete both checklists: Self-review before requesting review
  4. Test all worlds: Verify inheritance
  5. Read the guidelines: Encoding Guidelines

In the PR Description

Include:

## Summary
[What this encoding adds]

## Seif Encoded
[YD XX:Y]

## Rules Added
- r_rule_name_1: [description]
- r_rule_name_2: [description]

## Test Coverage
- Positive: X tests
- Negative: Y tests
- Edge cases: Z tests
- Multi-world: W tests

## Machloket (if applicable)
- Position 1: [authority] - [summary]
- Position 2: [authority] - [summary]

## Checklists
- [x] Technical checklist complete
- [x] Halachic checklist complete
- [x] All tests pass
- [x] Type checker clean

During Review

  • Respond promptly: Keep the review moving
  • Be thorough: Address every comment
  • Ask questions: If unclear, ask before guessing
  • Don't argue unnecessarily: Trust the reviewer's expertise
  • Learn from feedback: Apply lessons to future encodings

Summary

Successful review navigation requires:

  1. Preparation: Meet all requirements before submitting
  2. Self-review: Use checklists to catch issues early
  3. Responsiveness: Address feedback quickly and thoroughly
  4. Communication: Document changes and responses clearly
  5. Learning: Improve from each review experience

Quick Reference: Review Commands

# Pre-submission checks
pytest tests/                    # Run all tests
mistaber typecheck               # Check types
mistaber compile --check         # Verify compilation
ruff check .                     # Lint

# After review comments
git add -p                       # Stage changes incrementally
git commit -m "address review"   # Commit fixes
git push                         # Update PR

# Common responses
"Fixed. [Description of change]"
"Clarification needed: [Question]"
"Agreed. Changed X to Y per your suggestion."

Congratulations!

You have completed all five encoding tutorials. You now know how to:

  1. Execute the complete encoding workflow
  2. Handle machloket with override semantics
  3. Encode context-sensitive rules
  4. Write comprehensive test suites
  5. Navigate the review process

You are ready to contribute production-quality encodings to Mistaber.