feat: implement 10 missing domain features for ImpactGuard#8
Merged
feat: implement 10 missing domain features for ImpactGuard#8
Conversation
…tion, ABC cascade, type compat, re-exports, markdown, history, feedback, schema) Agent-Logs-Url: https://github.com/daedalus/ImpactGuard/sessions/b8048bc5-4f7d-41ee-aaac-a1bd76edb6a1 Co-authored-by: daedalus <115175+daedalus@users.noreply.github.com>
…, mutually exclusive flags, unused var) Agent-Logs-Url: https://github.com/daedalus/ImpactGuard/sessions/b8048bc5-4f7d-41ee-aaac-a1bd76edb6a1 Co-authored-by: daedalus <115175+daedalus@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
daedalus
May 6, 2026 01:33
View session
Reviewer's GuideAdds ten production-grade domain features to ImpactGuard: schema validation for all JSON contracts, suppression and all-aware visibility in signature extraction/comparison, deprecation lifecycle handling, type-widening classification, protocol/ABC cascade analysis, init.py re-export propagation, markdown PR report generation, tagged baseline history with CLI, feedback-driven risk calibration, and wires these through the public API and CLI with updated risk/semver scoring and comprehensive tests. Sequence diagram for the new history compare workflowsequenceDiagram
actor Developer
participant CLI_main as CLI_main
participant Baseline as BaselineModule
participant Extract as ExtractSignatures
participant Compare as CompareSignatures
participant Semver as SemverModule
Developer->>CLI_main: impactguard history compare <tag_from> [files]
CLI_main->>Baseline: compare_with_tagged_baseline(tag_from, files, history_path)
Baseline->>Baseline: load_tagged_baseline(tag, history_path)
Baseline->>Baseline: _load_history(effective_path)
Baseline-->>Baseline: old_signatures
Baseline->>Extract: extract(new_files)
Extract-->>Baseline: new_signatures
Baseline->>Baseline: write old.json and new.json to tempdir
Baseline->>Compare: compare(old_path, new_path, include_private)
Compare->>Compare: load(old_path)
Compare->>Compare: validate_signatures(data)
Compare->>Compare: load(new_path)
Compare->>Compare: validate_signatures(data)
Compare-->>Baseline: comparison{breaking,nonbreaking,suppressed}
Baseline->>Semver: format_semver_recommendation(comparison)
Semver-->>Baseline: semver_rec
Baseline-->>CLI_main: {comparison, semver_rec, baseline_tag, metadata}
CLI_main->>CLI_main: print counts and semver recommendation
alt output path provided
CLI_main->>CLI_main: json.dump(result, output)
end
CLI_main-->>Developer: exit_code (1 if breaking else 0)
Sequence diagram for the new feedback calibration workflowsequenceDiagram
actor Developer
participant CLI_main as CLI_main
participant Feedback as FeedbackModule
participant ConfigFile as ConfigFile
Developer->>CLI_main: impactguard feedback calibrate [--feedback-path] [--config-path]
CLI_main->>Feedback: load_outcomes(feedback_path)
Feedback-->>CLI_main: outcomes
CLI_main->>Feedback: compute_calibrated_weights(outcomes)
Feedback-->>CLI_main: weights
alt not enough data
CLI_main->>Developer: print "Not enough data for calibration"
CLI_main-->>Developer: exit_code 0
else sufficient data
CLI_main->>Feedback: apply_weights_to_config(weights, config_path)
Feedback->>ConfigFile: read existing impactguard.toml
Feedback->>Feedback: _upsert_toml_section(lines, impactguard.patches, weights)
Feedback->>ConfigFile: write updated impactguard.toml
Feedback-->>CLI_main: ok
alt write ok
CLI_main->>Developer: print calibrated weights summary
CLI_main-->>Developer: exit_code 0
else write failed
CLI_main->>Developer: print error to stderr
CLI_main-->>Developer: exit_code 1
end
end
Updated class diagram for core ImpactGuard analysis modulesclassDiagram
class ExtractSignatures {
+extract(files, base_path, include_reexports)
+extract_reexports(files)
+_has_ignore_comment(source_lines, lineno)
+_extract_all_names(tree)
+_unparse_annotation(node)
+arg_info(arg, default)
}
class CompareSignatures {
+load(path)
+compare(old_path, new_path, include_private)
+_is_public(fqname)
+_is_effectively_public(fqname, sig)
+_is_ignored(fqname, sig, suppress_list)
+_parse_union_members(type_str)
+_type_change_kind(old_type, new_type)
+_has_deprecated_decorator(sig)
}
class BaselineModule {
+DEFAULT_BASELINE_PATH
+DEFAULT_HISTORY_PATH
+save_baseline(files, path, metadata)
+load_baseline(path)
+compare_with_baseline(files, path, include_private)
+baseline_exists(path)
+save_tagged_baseline(tag, files, history_path, metadata)
+load_tagged_baseline(tag, history_path)
+list_baselines(history_path)
+compare_with_tagged_baseline(tag, new_files, history_path, include_private)
+delete_tagged_baseline(tag, history_path)
+_resolve_path(path)
+_resolve_history_path(path)
+_load_history(path)
}
class SchemaModule {
+validate_signatures(data)
+validate_calls(data)
+validate_runtime(data)
+validate_risk_report(data)
+validate(kind, data)
+_check_list(data, label, errors)
+_check_fields(item, required, label, idx, errors)
+_check_arg(arg, label, idx, errors)
}
class FeedbackModule {
+DEFAULT_FEEDBACK_PATH
+record_outcome(patch_id, accepted, change_type, patch_data, feedback_path)
+load_outcomes(feedback_path)
+get_stats(feedback_path)
+compute_calibrated_weights(outcomes)
+apply_weights_to_config(weights, config_path)
+_resolve_path(feedback_path)
+_load_raw(path)
+_save_raw(path, outcomes)
+_upsert_toml_section(lines, section_header, values)
}
class ClassHierarchyModule {
+ClassInfo
+Hierarchy
+extract_class_hierarchy(files)
+find_implementations(hierarchy)
+get_cascade_changes(comparison, hierarchy, implementations)
+_base_names(bases)
+_is_abstract(base_names)
}
class GenerateReportModule {
+generate_html(report_data)
+generate_html_from_file(risk_json_path, output_path)
+generate_markdown(report_data, semver_rec, max_rows)
+generate_markdown_from_file(risk_json_path, output_path, semver_rec)
+_summary_stats(report_data)
}
class RiskModelModule {
+CHANGE_WEIGHTS
}
class SemverModule {
+BREAKING_PREFIXES
+format_semver_recommendation(comparison)
}
ExtractSignatures ..> ClassHierarchyModule : provides_methods_for
BaselineModule ..> ExtractSignatures : uses_extract
BaselineModule ..> CompareSignatures : uses_compare
BaselineModule ..> SemverModule : uses_format_semver_recommendation
CompareSignatures ..> SchemaModule : uses_validate_signatures
GenerateReportModule ..> SemverModule : uses_semver_rec
FeedbackModule ..> RiskModelModule : calibrates_patch_weights
ClassHierarchyModule ..> CompareSignatures : analyzes_cascade_from
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 84 minor |
| Complexity | 16 medium |
🟢 Metrics 351 complexity
Metric Results Complexity 351
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
extract_class_hierarchy,methodsare collected viaast.walk(node)which will also include methods of nested classes defined inside this class; if you only intend direct methods on the class, consider iterating overnode.bodyand filteringFunctionDef/AsyncFunctionDefinstead. - In
schema._check_arg/validate_signatures, the index reported in error messages is always0for arguments (because_check_fieldsis called withidx=0), which makes debugging harder; consider threading the actual argument index through so positional and keyword-only argument errors can be correctly attributed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `extract_class_hierarchy`, `methods` are collected via `ast.walk(node)` which will also include methods of nested classes defined inside this class; if you only intend direct methods on the class, consider iterating over `node.body` and filtering `FunctionDef`/`AsyncFunctionDef` instead.
- In `schema._check_arg` / `validate_signatures`, the index reported in error messages is always `0` for arguments (because `_check_fields` is called with `idx=0`), which makes debugging harder; consider threading the actual argument index through so positional and keyword-only argument errors can be correctly attributed.
## Individual Comments
### Comment 1
<location path="src/impactguard/baseline.py" line_range="216-218" />
<code_context>
+ """
+ effective_path = _resolve_history_path(history_path)
+ if not Path(effective_path).is_file():
+ raise FileNotFoundError(
+ f"History file not found: {effective_path}. "
+ "Run `impactguard baseline save --tag <tag>` first."
+ )
+ history = _load_history(effective_path)
</code_context>
<issue_to_address>
**issue:** Error message suggests an outdated CLI command (`baseline save --tag`) instead of the new `history save` interface.
This message should direct users to `impactguard history save <tag> ...`, which is the current way to create tagged baselines. Please update the text (and arguments, if needed) so the guidance matches the new history interface when the file is missing.
</issue_to_address>
### Comment 2
<location path="src/impactguard/compare_signatures.py" line_range="73-82" />
<code_context>
+def _parse_union_members(type_str: str) -> frozenset[str]:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Union parsing is too naive for nested/complex type annotations and may misclassify type changes.
`_parse_union_members` currently splits on `","`, so `Union[tuple[int, int], str]` is parsed into four members instead of two. This can misclassify widening/narrowing and thus whether a change is breaking. To avoid this, parse with bracket-depth awareness or via the AST rather than raw string splitting.
</issue_to_address>
### Comment 3
<location path="src/impactguard/schema.py" line_range="35-40" />
<code_context>
+ errors.append(f"{label}[{idx}]: missing required field '{field}'")
+
+
+def _check_arg(arg: object, label: str, idx: int, errors: list[str]) -> None:
+ """Validate a single argument dict within a signature."""
+ if not isinstance(arg, dict):
+ errors.append(f"{label}[{idx}]: argument entry must be an object")
+ return
+ _check_fields(arg, ["name", "has_default"], f"{label}[{idx}].arg", 0, errors)
+
+
</code_context>
<issue_to_address>
**suggestion:** Argument index handling in validation error messages is confusing and likely incorrect.
In `_check_arg`, `idx` is used as the argument index but is always derived from the signature index in `validate_signatures`, and `_check_fields` is called with a hard‑coded index `0`. This produces diagnostics like `signatures[3].positional[3].arg[0]` that don’t match the real argument position. Consider passing the actual argument index into `_check_arg` and through to `_check_fields` so the reported paths reflect the true argument location.
Suggested implementation:
```python
# Use the actual argument index so diagnostics reflect the real argument location.
_check_fields(arg, ["name", "has_default"], label, idx, errors)
```
To fully align diagnostics with the true argument positions, you should also:
1. Update the place where `_check_arg` is called (likely in `validate_signatures`) so that:
- The `idx` argument passed to `_check_arg` is the *argument index* within the relevant list (e.g. `positional`, `keyword_only`, etc.), not the signature index.
- The `label` argument passed to `_check_arg` is the base path to the argument list (e.g. `f"signatures[{sig_idx}].positional"`), letting `_check_fields` compose `"...positional[{arg_idx}]"`.
2. Verify other callers of `_check_fields` are still passing a base label and the correct index for that collection, to keep diagnostic paths consistent.
</issue_to_address>
### Comment 4
<location path="tests/test_ten_features.py" line_range="884" />
<code_context>
+ validate("widgets", [])
+
+
+def test_validate_signatures_emits_warning_on_load(tmp_path: Path, capsys: pytest.CaptureFixture) -> None:
+ """compare_signatures.load() should warn to stderr on invalid data."""
+ from impactguard.compare_signatures import load
</code_context>
<issue_to_address>
**issue (testing):** The warning assertion is currently too weak and can pass even if no warning is emitted.
The current assertion `assert "Warning" in captured.err or len(captured.err) == 0` allows the test to pass even when no warning is printed, which contradicts the docstring and weakens coverage of `compare_signatures.load`’s integration with `validate_signatures`. Tighten this to require a warning (e.g. `assert "Warning: signatures file" in captured.err`), and if you need to assert that invalid data is non-fatal, add a separate test for the return value instead of relaxing the warning requirement here.
</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
+216
to
+218
| raise FileNotFoundError( | ||
| f"History file not found: {effective_path}. " | ||
| "Run `impactguard baseline save --tag <tag>` first." |
There was a problem hiding this comment.
issue: Error message suggests an outdated CLI command (baseline save --tag) instead of the new history save interface.
This message should direct users to impactguard history save <tag> ..., which is the current way to create tagged baselines. Please update the text (and arguments, if needed) so the guidance matches the new history interface when the file is missing.
Comment on lines
+73
to
+82
| def _parse_union_members(type_str: str) -> frozenset[str]: | ||
| """Break a type annotation string into its constituent member types. | ||
|
|
||
| Handles: | ||
|
|
||
| * PEP 604 ``X | Y | Z`` | ||
| * ``Optional[X]`` → ``{X, None}`` | ||
| * ``Union[X, Y]`` → ``{X, Y}`` | ||
| * Everything else → ``{type_str}`` | ||
| """ |
There was a problem hiding this comment.
suggestion (bug_risk): Union parsing is too naive for nested/complex type annotations and may misclassify type changes.
_parse_union_members currently splits on ",", so Union[tuple[int, int], str] is parsed into four members instead of two. This can misclassify widening/narrowing and thus whether a change is breaking. To avoid this, parse with bracket-depth awareness or via the AST rather than raw string splitting.
Comment on lines
+35
to
+40
| def _check_arg(arg: object, label: str, idx: int, errors: list[str]) -> None: | ||
| """Validate a single argument dict within a signature.""" | ||
| if not isinstance(arg, dict): | ||
| errors.append(f"{label}[{idx}]: argument entry must be an object") | ||
| return | ||
| _check_fields(arg, ["name", "has_default"], f"{label}[{idx}].arg", 0, errors) |
There was a problem hiding this comment.
suggestion: Argument index handling in validation error messages is confusing and likely incorrect.
In _check_arg, idx is used as the argument index but is always derived from the signature index in validate_signatures, and _check_fields is called with a hard‑coded index 0. This produces diagnostics like signatures[3].positional[3].arg[0] that don’t match the real argument position. Consider passing the actual argument index into _check_arg and through to _check_fields so the reported paths reflect the true argument location.
Suggested implementation:
# Use the actual argument index so diagnostics reflect the real argument location.
_check_fields(arg, ["name", "has_default"], label, idx, errors)To fully align diagnostics with the true argument positions, you should also:
- Update the place where
_check_argis called (likely invalidate_signatures) so that:- The
idxargument passed to_check_argis the argument index within the relevant list (e.g.positional,keyword_only, etc.), not the signature index. - The
labelargument passed to_check_argis the base path to the argument list (e.g.f"signatures[{sig_idx}].positional"), letting_check_fieldscompose"...positional[{arg_idx}]".
- The
- Verify other callers of
_check_fieldsare still passing a base label and the correct index for that collection, to keep diagnostic paths consistent.
| validate("widgets", []) | ||
|
|
||
|
|
||
| def test_validate_signatures_emits_warning_on_load(tmp_path: Path, capsys: pytest.CaptureFixture) -> None: |
There was a problem hiding this comment.
issue (testing): The warning assertion is currently too weak and can pass even if no warning is emitted.
The current assertion assert "Warning" in captured.err or len(captured.err) == 0 allows the test to pass even when no warning is printed, which contradicts the docstring and weakens coverage of compare_signatures.load’s integration with validate_signatures. Tighten this to require a warning (e.g. assert "Warning: signatures file" in captured.err), and if you need to assert that invalid data is non-fatal, add a separate test for the return value instead of relaxing the warning requirement here.
daedalus
added a commit
that referenced
this pull request
May 7, 2026
- Fix malformed function names in risk report (Gap #1 introduced) risk_gate.run() now extracts fqname before first space Fixes TYPE_CHANGED and DECORATOR_REMOVED entries seen_functions now uses fqname (not malformed string) Runtime lookup now works correctly for these change types - Fix DECORATOR_ADDED contradiction (Gap #3) compare_signatures.py now appends to nonbreaking (not breaking) Consistent with risk_model.py severity 0.1 (typically non-breaking) - Fix README API mismatch (Gap #6) Updated run_pipeline() examples to use old_files/new_files Matches actual function signature in pipeline.py - Delete requirements.txt (Gap #8) Redundant with pyproject.toml, was misleading Added note in requirements.txt pointing to pyproject.toml - Fix AGENTS.md Python version (Gap #9) Updated from 3.9 to 3.11 to match pyproject.toml - Fix transitive_depth default (Gap #12) Changed default from 0 to 1 in config.py Enables transitive impact tracking by default All 1223 tests pass with 80.99% coverage
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.
ImpactGuard lacked several features fundamental to a production-grade API impact analyzer: no way to suppress known-intentional changes, no
__all__-aware visibility, no deprecation lifecycle, no type-compatibility reasoning, no Protocol/ABC cascade propagation, no re-export tracking, no markdown CI output, no release-history baselines, no feedback calibration loop, and no data contract validation.New modules
schema.py— Structural validators for all inter-module JSON formats (validate_signatures,validate_calls,validate_runtime,validate_risk_report). Integrated intocompare_signatures.load()as non-fatal warnings.class_hierarchy.py—extract_class_hierarchy/find_implementations/get_cascade_changes: detects Protocol/ABC bases and surfaces cascade impact when their methods change.feedback.py—record_outcome,get_stats,compute_calibrated_weights,apply_weights_to_config: records patch acceptance/rejection outcomes and calibrates[impactguard.patches]weights back toimpactguard.toml.Modified modules
extract_signatures.pyignored: boolfield — set when# impactguard: ignoreappears on/before thedeflineexported: bool | Nonefield —True/Falsewhen__all__is defined,Noneotherwiseextract_reexports(files)— parses__init__.pyrelative imports into{public_fqname: source_fqname}extract(..., include_reexports=True)— appends alias signatures for re-exported namescompare_signatures.pyignored=Trueor matching[impactguard.analysis] suppresslist are skipped; result now includessuppressedkey__all__visibility:_is_effectively_public()gates onexportedfield before falling back to underscore heuristic@deprecated*decorator emitDEPRECATED REMOVED:intononbreakinginstead ofbreaking_type_change_kind(old, new)classifies widening (int → int | None) as nonbreakingTYPE WIDENED; narrowing/changed stays breakingTYPE CHANGEDgenerate_report.pygenerate_markdown(report_data, semver_rec, max_rows)— compact risk table + badge row for GitHub PR commentsgenerate_markdown_from_file(path, output_path, semver_rec)— file-based variantbaseline.pysave_tagged_baseline(tag, files, ...)/load_tagged_baseline(tag)/list_baselines()/compare_with_tagged_baseline(tag, new_files)/delete_tagged_baseline(tag)— append-log of named snapshots in.impactguard_history.jsonrisk_model.py/semver.pyDEPRECATED REMOVED: 0.15,TYPE WIDENED: 0.05,RETURN TYPE WIDENED: 0.05severity entriesDEPRECATED REMOVEDexcluded from semver major-bump triggersCLI additions
impactguard report-markdown <report.json>impactguard feedback record <id> --accepted|--rejectedimpactguard feedback statsimpactguard feedback calibrateimpactguard history save <tag>impactguard history listimpactguard history compare <tag>impactguard history delete <tag>Summary by Sourcery
Add suppression, visibility, lifecycle, type-compatibility, hierarchy, reporting, baseline history, and feedback-loop capabilities to ImpactGuard’s core API analysis pipeline.
New Features:
Enhancements:
Tests: