Feature/normalization #104
Conversation
AlexanderOnischenko
force-pushed
the
feature/normalization
branch
from
d8fdd54 to
b0068b0
Compare
This reverts commit c737c77.
|
@codex, please review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex, please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 587196f79d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.options.add_required_context_specs: | ||
| context_plan = self._ensure_required_specs( | ||
| required_context, context_plan, notes | ||
| ) |
There was a problem hiding this comment.
When add_required_context_specs is enabled, normalize() injects default ContextFetchSpec entries for every required_context, even if the plan originally had no context_plan. Because _merge_baseline_with_plan later treats plan.context_plan as authoritative (it assigns by_provider[s.provider] = s), these auto-injected specs overwrite baseline entries for the same provider, dropping any baseline selectors/mode. This regression shows up when a plan only declares required_context (no explicit specs) and a baseline is meant to supply required selectors: after normalization, the empty default spec replaces the baseline and the fetch happens without the required constraints.
Useful? React with 👍 / 👎.
…etdefault and normalize text lists (#105) * Do not synthesize required_context in PlanNormalizer; always merge required_context via `setdefault` ### Motivation - Prevent silently overwriting baseline `ContextFetchSpec` entries by stopping the normalizer from fabricating specs for `required_context` and moving that responsibility to the merge layer. - Ensure baseline-aware materialization of missing providers so baseline selectors and modes are preserved. - Reduce confusing public API surface by removing now-dead `PlanNormalizerOptions` flags related to required-context synthesis. ### Description - Stop synthesizing `ContextFetchSpec` from `required_context` in `PlanNormalizer.normalize()` and add an explanatory comment that merge logic must be baseline-safe. - Remove the `fill_missing_context_plan` and `add_required_context_specs` fields from `PlanNormalizerOptions`. - Change `BaseGraphAgent._merge_baseline_with_plan` to always `setdefault` entries from `plan.required_context`, materializing missing providers as `ContextFetchSpec(provider=p, mode="full")` while preserving any baseline specs. - Add `tests/test_context_merge.py` with three regression tests exercising baseline preservation and required-context materialization, and make the materialization test less brittle by asserting individual fields rather than a full `model_dump`, and explicitly use an empty baseline list for clarity. ### Testing - Added unit tests in `tests/test_context_merge.py` covering baseline preservation when `context_plan` is missing or empty and materialization of `required_context` when no baseline exists. - An attempted test run (`pytest -k context_merge`) failed during collection due to an unrelated `ImportError: cannot import name 'NotRequired' from 'typing'` in `examples/demo_qa` on Python 3.10, so the new tests were not executed in this environment. - No further automated tests were run in this update. * Stop PlanNormalizer synthesizing required_context; always merge via setdefault and normalize text lists ### Motivation - Prevent overwriting baseline `ContextFetchSpec` entries by moving required-provider synthesis out of the normalizer and into the baseline/plan merge logic. - Ensure downstream code and templates always receive list values for `adr_queries` and `constraints` so they never get `None`. - Add regression coverage to guarantee that `required_context` is materialized even when `context_plan` is non-empty. ### Description - Removed synthesis of `ContextFetchSpec` from `PlanNormalizer.normalize()` and added a comment that baseline/plan merge must handle required providers in a baseline-safe way. - Removed unused flags from `PlanNormalizerOptions` related to filling/mutating `context_plan` and `required_context`. - Changed `BaseGraphAgent._merge_baseline_with_plan` to always `setdefault` entries from `plan.required_context`, materializing missing providers as `ContextFetchSpec(provider=p, mode="full")` while preserving any baseline specs. - Updated `PlanNormalizer._normalize_text_list` to always return a `List[str]` (returning an empty list when input is `None`), and used it for `adr_queries` and `constraints` so normalized plans never contain `None` for those fields. - Added `tests/test_context_merge.py` with four regression tests covering baseline preservation and required_context materialization, including the case where `context_plan` is non-empty. ### Testing - Added unit tests in `tests/test_context_merge.py` covering baseline preservation when `context_plan` is missing or empty, materialization of `required_context` when no baseline exists, and materialization when `context_plan` is non-empty. - No automated tests were executed as part of this update, so there are no test run results to report.
|
@codex, please review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Нормализация селекторов