fix: Jinja template crash on phases_completed=None, submodule updates#113
Open
ElNiak wants to merge 56 commits intoproductionfrom
Open
fix: Jinja template crash on phases_completed=None, submodule updates#113ElNiak wants to merge 56 commits intoproductionfrom
ElNiak wants to merge 56 commits intoproductionfrom
Conversation
Updates panther_ivy to feat/mcp-tool-fixes-and-requirements which includes: - Fixed 6 issues in ivy-tools MCP server (ISS-001/002/003/005/006/008) - All 19 MCP tools verified working via live calls - Added RFC 9000 requirements manifest (101 requirements) - Auto-detect local ivy-lsp source for development
Update panther_ivy submodule with the new Pattern Library featuring: - 13 reusable .ivy template files for 7 formal patterns - Pattern detection engine with cross-reference validation - 2 new MCP tools (ivy_pattern_analysis, ivy_pattern_scaffold) - /nct-add-pattern command and pattern-library skill - Enhanced coverage gaps, smart suggestions, and LSP diagnostics
Picks up ivy-lsp changes: semantic edge wiring (COVERS, HAS_PARAM, RETURNS_TYPE, INCLUDES), smart suggestions fix, per-action state var filtering, and three new MCP tools (ivy_generate_manifest, ivy_scaffold_check, ivy_quality_gate).
Requirements YAML cleanup, ivy-lsp bug fixes, plugin hook hardening.
Batch 1 (P0): Fix broken wiring - Fix api/ broken relative imports (api/_shared.py) - Wire all 25 MCP tools into Claude Code plugin - Update ivy-tooling-guide skill with complete tool mapping Batch 2 (P1): Quality debt - Modularize mcp_server.py (2225 → 728 lines + tools/ package) - Deduplicate shell script workspace detection (workspace-common.sh) - Fix PantherIvyVersion docstring (class is used, not dead code)
…lignment) Python tester: remove shadowed adapt_environment_paths, consolidate 3 protocol name methods to 1, merge duplicate output patterns, remove dead build_tests(). Plugin: fix README inventory counts, CSO-optimize all 15 skill descriptions, add Red Flags/Integration/Common Mistakes sections to process skills, normalize skill name casing to kebab-case.
Update panther_ivy submodule with MCP tool consolidation (25→15), plugin surface reduction (agents 9→4, skills 14→6, commands 6→5), counterexample parser, per-isolate caching, and coverage diff mode. Add strategic evaluation document comparing Ivy tooling to state of the art across 5 dimensions (code intelligence, verification, traceability, AI-assisted specification, protocol analysis).
ivy-lsp: fix verify cache race, diagnostic layer errors, model init retry, compile path bug, pattern mode validation, logging, quality gate OSError, coverage diff truncation, off-by-one in pattern library, stale tool names in tests. Add counterexample parser, verification cache, and traceability tool tests. panther-ivy-plugin: fix shell injection in hooks, JSON parsing in hooks, brace counting with comment stripping, stale MCP tool names in agents/ skills/commands/READMEs, component counts and version.
Updates submodule pointers for panther-ivy-plugin and ivy-lsp with fixes for all 18 issues from the plugin evaluation report: Plugin docs: tool name alignment, skill/agent listing, plugin.json fields, version bump to 0.5.0, Stop hook for session summary. Server: cross-reference fuzzy matching, symbol disambiguation with protocol scoping, coverage tag normalization, hover SemanticModel fallback, test_file filtering, output size limits, workDoneProgress capability guard, individual tool aliases for backward compatibility.
…iew fixes) Evaluation doc: - TLA+: "Shipping (SANY-based)" not "In development" - ProVerif: "Partial" not "Full" LSP - Tamarin: note web prover provides exploration Submodule (panther_ivy): - Fix classify_endpoint_type vs _extract_test_directory_from_name divergence - Fix docstrings, add edge case tests, clean unused imports - Fix pattern catalog detection markers
LSP: 17→19 features (add implementation, call_hierarchy to Appendix B). Patterns: 6→7 types (add include-chain). Code metrics: analysis_pipeline 874→896 lines, 37→~17 methods; model.py 252→295 lines; requirement_graph 400+→718 lines. Note backward-compatibility aliases in Appendix A.
- Fix _extract_test_directory divergence from classify_endpoint_type - Re-export classify_endpoint_type in api/_shared.py - Remove 4 duplicate entries from rfc9000_requirements.yaml
…tale comments) - Revert autoescape=True to False in Jinja env (Markdown, not HTML) - Switch hardcoded 1/2/3 numbering to bullet points in failure analysis - Add missing Phases column to fallback markdown reporter - Restore delegation-pattern "why" context on emit comments - Fix stale StateManager reference and wrong progress bar comment - Remove redundant inline comment in metrics_observer - Document test_name retention and timing-includes-init behavior - Add file references for backward-compat alias count in eval doc - Fix pre-existing D212 docstring style warnings in experiment_reporter
The experiment_report.md.jinja template called .values() on phases_completed without checking for None, causing TypeError when services lack phase data (the common default case). This made the Jinja template path silently fall back to basic markdown. Also updates panther_ivy submodule with: - rfc9000 requirements section-key alignment (6 mismatches) - ivy-lsp submodule (gitignore for temp dir leakage) - panther-ivy-plugin submodule (LSP env config in marketplace.json, version bump, hook matcher scoping)
…on doc - Tool count: 15→17 (adds ivy_verification_dashboard, ivy_generate_manifest) - Layer list: corrected to match tools/patterns.py canonical 14-layer template - Line references: updated mcp_server.py, requirement_graph.py, pattern_library.py, compiler_adapter.py line numbers; removed bogus visualization.py:602 - Added [RESOLVED] annotations for skills/agents created in this PR
…lidated tool names)
…coping, ground truth update)
Contributor
Reviewer's GuideFixes crashes in the Jinja experiment report when phases_completed is None, aligns the basic markdown report with the Jinja output, clarifies observer/manager/reporting comments, disables Jinja autoescaping for markdown templates, and updates the panther_ivy tester submodule plus adds Ivy tooling evaluation documentation. Sequence diagram for experiment report generation with Jinja and fallbacksequenceDiagram
actor User
participant ExperimentManager
participant ExperimentReporter
participant JinjaEnv
participant Filesystem as OutputFile
User->>ExperimentManager: run_experiment()
ExperimentManager->>ExperimentReporter: generate_reports()
alt jinja_env_available
ExperimentReporter->>JinjaEnv: render(experiment_report_template, summary_with_services)
alt phases_completed_is_dict
JinjaEnv-->>ExperimentReporter: rendered_markdown with "done/total" phases
else phases_completed_is_none_or_empty
JinjaEnv-->>ExperimentReporter: rendered_markdown with "N/A" phases
end
ExperimentReporter->>Filesystem: write(markdown_report)
Filesystem-->>ExperimentReporter: success
ExperimentReporter-->>ExperimentManager: {experiment_report: True}
else jinja_env_not_available
ExperimentReporter->>ExperimentReporter: _generate_basic_markdown_report(summary)
alt phases_completed_is_dict
ExperimentReporter->>ExperimentReporter: compute done = sum(values)
ExperimentReporter->>ExperimentReporter: phases = "done/total"
else phases_completed_is_none_or_empty
ExperimentReporter->>ExperimentReporter: phases = "N/A"
end
ExperimentReporter->>Filesystem: write(basic_markdown_report)
Filesystem-->>ExperimentReporter: success
ExperimentReporter-->>ExperimentManager: {experiment_report: True}
end
ExperimentManager-->>User: experiment completed, report available
Updated class diagram for ExperimentReporter and ServiceHealthSummaryclassDiagram
class ExperimentReporter {
-Path experiment_dir
-Optional~str~ experiment_name
-Optional~Environment~ jinja_env
-Logger logger
+ExperimentReporter(experiment_dir, experiment_name)
+generate_reports() Dict~str, bool~
-_generate_basic_markdown_report(summary) bool
+generate_quick_summary() Optional~str~
}
class ServiceHealthSummary {
+str service_name
+str status
+bool compilation_succeeded
+Optional~Dict~str, bool~~ phases_completed
+Optional~str~ error_summary
+float output_completeness
+Optional~str~ test_name
+to_dict() Dict~str, Any~
}
ExperimentReporter --> ServiceHealthSummary : uses in reports
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In both the Python fallback renderer and the Jinja template,
if svc.phases_completedwill treat an empty dict as falsy and show"N/A"; if an empty phases dict is a valid state, you may want to distinguishNonefrom{}so that 0/N can be rendered instead ofN/A. - The phases formatting logic (
done/totalvsN/A) is now duplicated between_generate_basic_markdown_reportand the Jinja template; consider centralizing this into a helper or precomputed field onServiceHealthSummaryto keep behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both the Python fallback renderer and the Jinja template, `if svc.phases_completed` will treat an empty dict as falsy and show `"N/A"`; if an empty phases dict is a valid state, you may want to distinguish `None` from `{}` so that 0/N can be rendered instead of `N/A`.
- The phases formatting logic (`done/total` vs `N/A`) is now duplicated between `_generate_basic_markdown_report` and the Jinja template; consider centralizing this into a helper or precomputed field on `ServiceHealthSummary` to keep behavior consistent.
## Individual Comments
### Comment 1
<location path="panther/core/reporting/experiment_reporter.py" line_range="147-149" />
<code_context>
trim_blocks=True,
lstrip_blocks=True,
- autoescape=True,
+ # Templates produce Markdown, not HTML — autoescape would
+ # corrupt <, >, & in variable content.
+ autoescape=False,
)
else:
</code_context>
<issue_to_address>
**🚨 issue (security):** Disabling Jinja autoescaping for Markdown may open up injection risks for untrusted content.
This fixes `<`, `>` and `&` corruption, but also renders any user-controlled values (e.g. `error_summary`, test names, log paths) completely unescaped. If these Markdown reports are ever rendered as or converted to HTML, that becomes an injection vector. Instead, consider `select_autoescape(['html', 'xml'])` so `.md` stays unescaped, or restrict which fields may contain raw markup and escape the rest explicitly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+147
to
+149
| # Templates produce Markdown, not HTML — autoescape would | ||
| # corrupt <, >, & in variable content. | ||
| autoescape=False, |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Disabling Jinja autoescaping for Markdown may open up injection risks for untrusted content.
This fixes <, > and & corruption, but also renders any user-controlled values (e.g. error_summary, test names, log paths) completely unescaped. If these Markdown reports are ever rendered as or converted to HTML, that becomes an injection vector. Instead, consider select_autoescape(['html', 'xml']) so .md stays unescaped, or restrict which fields may contain raw markup and escape the rest explicitly.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes experiment report generation when a service has phases_completed=None, and updates the panther_ivy submodule to a newer revision with multiple tooling and matching fixes.
Changes:
- Guard
phases_completedaccess in the Jinja experiment report template to avoid crashing onNone. - Expand the basic (non-Jinja) Markdown report to include a “Phases” column with
N/Afallback. - Bump
panther_ivysubmodule pointer to a newer commit.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| panther/plugins/services/testers/panther_ivy | Updates the panther_ivy git submodule commit reference. |
| panther/core/reporting/templates/experiment_report.md.jinja | Adds a None guard for phases_completed and adjusts the “failed experiment” Markdown list formatting. |
| panther/core/reporting/status_collector.py | Comment clarifications around serialization and test timing extraction. |
| panther/core/reporting/experiment_reporter.py | Disables Jinja autoescaping; adds “Phases” to the basic Markdown report output and computes phase progress. |
| panther/core/observer/impl/metrics_observer.py | Removes a redundant comment. |
| panther/core/observer/impl/experiment_observer.py | Doc/comment clarifications about observer behavior and progress tracking. |
| panther/core/experiment_manager.py | Comment clarifications about responsibility for emitting lifecycle events. |
| docs/superpowers/specs/2026-03-13-ivy-tooling-evaluation.md | Adds a new tooling evaluation spec document. |
You can also share your feedback on Copilot code review. Take the survey.
| |---------|--------|-------------|--------|-----------|--------| | ||
| {% for svc in iut_services %} | ||
| | {{ svc.service_name }} | {{ svc.status }} | {{ 'OK' if svc.compilation_succeeded else 'FAIL' }} | {{ svc.phases_completed.values()|select|list|length }}/{{ svc.phases_completed|length }} | {{ svc.exit_code if svc.exit_code is not none else 'N/A' }} | {{ svc.error_summary or 'None' }} | | ||
| | {{ svc.service_name }} | {{ svc.status }} | {{ 'OK' if svc.compilation_succeeded else 'FAIL' }} | {{ (svc.phases_completed.values()|select|list|length ~ '/' ~ svc.phases_completed|length) if svc.phases_completed else 'N/A' }} | {{ svc.exit_code if svc.exit_code is not none else 'N/A' }} | {{ svc.error_summary or 'None' }} | |
| |---------|--------|-------------|--------|-----------|--------| | ||
| {% for svc in tester_services %} | ||
| | {{ svc.service_name }} | {{ svc.status }} | {{ 'OK' if svc.compilation_succeeded else 'FAIL' }} | {{ svc.phases_completed.values()|select|list|length }}/{{ svc.phases_completed|length }} | {{ svc.exit_code if svc.exit_code is not none else 'N/A' }} | {{ svc.error_summary or 'None' }} | | ||
| | {{ svc.service_name }} | {{ svc.status }} | {{ 'OK' if svc.compilation_succeeded else 'FAIL' }} | {{ (svc.phases_completed.values()|select|list|length ~ '/' ~ svc.phases_completed|length) if svc.phases_completed else 'N/A' }} | {{ svc.exit_code if svc.exit_code is not none else 'N/A' }} | {{ svc.error_summary or 'None' }} | |
Comment on lines
+147
to
+149
| # Templates produce Markdown, not HTML — autoescape would | ||
| # corrupt <, >, & in variable content. | ||
| autoescape=False, |
Comment on lines
+331
to
+337
| if svc.phases_completed: | ||
| done = sum( | ||
| 1 for v in svc.phases_completed.values() if v | ||
| ) | ||
| phases = f"{done}/{len(svc.phases_completed)}" | ||
| else: | ||
| phases = "N/A" |
Comment on lines
+137
to
+139
| - **Fast-fail was triggered**: {{ summary.fast_fail.reason }} | ||
| {%- if summary.fast_fail.error_category %} | ||
| - Error category: {{ summary.fast_fail.error_category }} | ||
| - Error category: {{ summary.fast_fail.error_category }} |
Comment on lines
+145
to
+147
| - **Failed tests to investigate**: | ||
| {%- for test in failed_tests %} | ||
| - {{ test.name }}: Check `./{{ test.logs_path }}/test.log` | ||
| - {{ test.name }}: Check `./{{ test.logs_path }}/test.log` |
Comment on lines
+151
to
154
| - **Next steps**: | ||
| - Review the main experiment log: `./experiment.log` | ||
| - Check individual test logs for detailed error information | ||
| - Verify configuration parameters in `experiment_config.yaml` |
…names, observability)
…eview simplifications)
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
Analyzes 7 problem categories found in conversation + debug logs: - SIGTERM crash from PID cleanup race condition - JSONDecodeError killing stdio LSP - 7-minute first-call latency (lazy model build) - 40% cross-validation disagreement between MCP and LSP - "server is running" busy state errors Three-phase fix plan: crash prevention, resilience, correctness.
Address 11 review findings (3 blocking, 3 medium, 5 low): - Fix 1c: correct pygls patch target (pygls_patches.py, not data_received) - Fix 2d: include freshness_key parameter in write_model_cache call - Fix 2a: note FastMCP lifecycle constraints for pre-warming - Fix 1a: clarify $PPID mechanism, CLAUDE_SESSION_ID doesn't exist - Fix 1b: add asyncio signal handler guidance for MCP mode - Fix 3b: clarify workspaceSymbol is query-based, not cursor-based - Fix 3c: note ivy_query was removed, fix targets semantic model - Phase 3: explicitly note sub-investigation requirement - Phasing table: clarify 2b is docs-only
- ivy-lsp: tool consolidation, MCP HTTP sidecar, stdio↔HTTP bridge - panther-ivy-plugin: consolidate plugins, mcp-bridge mode, skill updates
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
Adds a PreToolUse hook that warns Claude not to call the LSP tool directly on .ivy files, redirecting to MCP tools and Read/Grep/Glob.
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
Design for three-layer architecture: - Offline index builder (ivy-lsp index CLI) - Unified WorkspaceContext replacing 3 detection paths - Session overlay with multiple concurrent test scope views Key decisions: protocols are self-contained (APT has forked QUIC copies), per-protocol .ivy-index/ with full parser output, Tier 1 (AST) by default, backward-compatible migration path.
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
Design for migrating all 18 ivy-lsp MCP tools to Serena as the single MCP gateway, backed by a shared ivy_tools library extracted from ivy-lsp. Phased approach (0-5) with explicit exit criteria. Key decisions: - ivy_tools is the foundational library (no ivy_lsp dependency) - Three-layer sharing: code, disk cache (.ivy-index), live LSP - 20 Serena tools total (18 new + 2 existing LSP wrappers) - ivy-lsp MCP sidecar removed after migration complete
|
✅ PR Validation Passed Your changes look good! The quick validation checks have passed:
The full CI pipeline will run when this PR is merged or when targeting the main branches. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
phases_completed=Noneinexperiment_report.md.jinja(lines 57, 66) —.values()was called onNonewhen services lack phase data, causing TypeError and silent fallback to basic markdown rendererpanther_ivysubmodule with:Test plan
phases_completed=None(returns "N/A")pytest tests/unit/ -n auto -m unit— expect 1084+ passed, no new regressionsclassify_endpoint_type("quic_client_test_0rtt_mim_replay", "server")returns"client"Summary by Sourcery
Handle missing phase data safely in experiment reports, adjust Jinja environment for Markdown output, and update Ivy-related documentation and comments for clarity.
Bug Fixes:
Enhancements:
Documentation: