feat(agents): add ADR cross-reference table to AGENTS.md#246
feat(agents): add ADR cross-reference table to AGENTS.md#246KooshaPari wants to merge 11 commits into
Conversation
…-110, ADR-031) Per ADR-031 and the L5-104 §4 Step 3 plan, the canonical Rust config substrate is now KooshaPari/Configra. Two embedded sub-crates in this repo were either stale (pointed to phenoShared) or had no canonical marker at all. This commit adds the missing CANONICAL.md markers, re-pointing them to Configra. Markers added: - crates/phenotype-config-loader/CANONICAL.md — generic JSON/TOML loaders (load_json<T>, load_toml<T>, ConfigLoadError). Re-pointed to KooshaPari/Configra:crates/pheno-config/ for the canonical type-gated Config + load_from_file/load_from_toml_file. Generic T loading is a no-direct-replacement; consumers should open a Configra PR if they need it. - crates/phenotype-shared-config/CANONICAL.md — SDK helpers (ConfigSource, ConfigValue, SourcePriority, ConfigFormat, ConfigMeta, search_config_dirs, ConfigError). Re-pointed to KooshaPari/Configra:crates/pheno-config/ for the canonical Config struct + ConfigError. ConfigSource/ConfigValue/etc. have no direct replacement (intentional, no current consumer). This completes the L5-104 plan §4 Step 3. The remaining GitHub-level work (5 PRs on KooshaPari/Configra and KooshaPari/phenotype-config) is already merged (PRs #44, #45, #46, #47, #48). Refs: ADR-031, ADR-022, L5-104 §4 Step 3 Findings: findings/2026-06-18-L5-110-adr-035-impl.md
The standalone pheno-errors crate (KooshaPari/pheno-errors) is superseded by phenotype-error-core. All 5 AppError variants have direct equivalents in the existing layered error types. Documented in CANONICAL.md. Part of pheno-errors → phenotype-error-core absorption.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
More reviews will be available in 50 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
Note
|
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| #[derive(Debug, Error, PartialEq, Eq)] | ||
| pub enum FlagError { |
There was a problem hiding this comment.
Suggestion: Derive Clone for this public error enum so it follows the repository convention that public types implement Debug and Clone when feasible. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
FlagError is a public type and its variant payload is a String, so it can reasonably implement Clone. The enum derives Debug but not Clone, which matches the custom rule violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/phenotype-flags/src/lib.rs
**Line:** 72:73
**Comment:**
*Custom Rule: Derive `Clone` for this public error enum so it follows the repository convention that public types implement `Debug` and `Clone` when feasible.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::collections::BTreeMap; |
There was a problem hiding this comment.
Suggestion: Add FR traceability comments (// Traces to: FR-...) for the changed tests, starting with a module-level reference and per-test references where required by repository policy. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The file adds a new inline test module with multiple tests, but there is no // Traces to: FR-... comment at the module level or on the changed tests. This directly violates the repository rule requiring added or modified tests to include FR traceability references.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/phenotype-flags/src/lib.rs
**Line:** 222:225
**Comment:**
*Custom Rule: Add FR traceability comments (`// Traces to: FR-...`) for the changed tests, starting with a module-level reference and per-test references where required by repository policy.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
|
|
||
| # Generate coverage + open HTML in default browser | ||
| coverage-open: coverage-local | ||
| open coverage/html/index.html |
There was a problem hiding this comment.
Suggestion: The coverage-open recipe hardcodes the macOS open command, which fails on Linux/Windows environments with a command-not-found error. This makes the target non-functional for most contributors and inconsistent with the "default browser" intent; use OS-aware command selection (e.g., open/xdg-open/start) or a cross-platform launcher. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ `just coverage-open` fails on Linux developer machines.
- ⚠️ Coverage HTML must be opened manually by developers.Steps of Reproduction ✅
1. On a non-macOS environment (e.g., Linux), in the `pheno` repo root, run `just
coverage-open`, which invokes the `coverage-open` recipe defined in `justfile:52-53`.
2. The `coverage-open` recipe first runs its dependency `coverage-local` at
`justfile:46-49` to generate the HTML report under `coverage/html/index.html`.
3. After `coverage-local` completes, `just` executes the final recipe line `open
coverage/html/index.html` at `justfile:53`.
4. On Linux/Windows shells where the `open` command does not exist, this command fails
with a "command not found" error and a non-zero exit code, so `just coverage-open` does
not open the report in the default browser as documented.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** justfile
**Line:** 53:53
**Comment:**
*Api Mismatch: The `coverage-open` recipe hardcodes the macOS `open` command, which fails on Linux/Windows environments with a command-not-found error. This makes the target non-functional for most contributors and inconsistent with the "default browser" intent; use OS-aware command selection (e.g., `open`/`xdg-open`/`start`) or a cross-platform launcher.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| // happy on the `InvalidValue(env_key)` error path below. | ||
| let mut parsed: Vec<(String, bool)> = Vec::new(); | ||
| let mut offending: Option<String> = None; | ||
| for (env_key, env_value) in std::env::vars() { |
There was a problem hiding this comment.
Suggestion: from_env iterates with std::env::vars(), which can panic if any process environment key or value is not valid UTF-8 (common on Unix). That breaks the Result-based API contract by crashing instead of returning FlagError. Use std::env::vars_os() and handle non-UTF8 entries explicitly (skip or return a structured error) so this path cannot panic at runtime. [logic error]
Severity Level: Major ⚠️
- ❌ Process using FlagSet::from_env can panic on bad env.
- ⚠️ Error handling for feature flags becomes less predictable.Steps of Reproduction ✅
1. Use the documented environment-loading API `FlagSet::from_env("MYAPP")` as shown in the
crate docs example at `crates/phenotype-flags/src/lib.rs:37-45` (e.g., in a binary or
integration test).
2. Run this code on a Unix-like system where the OS process environment contains at least
one variable whose key or value is not valid UTF-8 (this is allowed by the OS and is
exactly the situation described in `std::env::vars` documentation).
3. When `FlagSet::from_env` executes the loop `for (env_key, env_value) in
std::env::vars()` at `crates/phenotype-flags/src/lib.rs:154`, the `vars()` iterator
attempts to convert each OS (key, value) to `String`.
4. For the first non-UTF-8 entry, `std::env::vars()` panics during iteration instead of
yielding a `Result`, so `FlagSet::from_env` never reaches the
`Err(FlagError::InvalidValue(..))` path at `crates/phenotype-flags/src/lib.rs:181-183`,
and the entire process aborts despite the function's `Result<Self, FlagError>` contract.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/phenotype-flags/src/lib.rs
**Line:** 154:154
**Comment:**
*Logic Error: `from_env` iterates with `std::env::vars()`, which can panic if any process environment key or value is not valid UTF-8 (common on Unix). That breaks the `Result`-based API contract by crashing instead of returning `FlagError`. Use `std::env::vars_os()` and handle non-UTF8 entries explicitly (skip or return a structured error) so this path cannot panic at runtime.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| unsafe { | ||
| std::env::set_var("UNRELATED_VAR", "true"); | ||
| std::env::set_var("MYAPP_FOO", "1"); | ||
| }; | ||
| let result = FlagSet::from_env("MYAPP"); | ||
| unsafe { | ||
| std::env::remove_var("UNRELATED_VAR"); | ||
| std::env::remove_var("MYAPP_FOO"); | ||
| }; |
There was a problem hiding this comment.
Suggestion: These tests mutate global process environment variables without any serialization, so parallel test execution can race across threads and produce flaky failures (or undefined behavior under Rust's unsafe env mutation contract). Guard env mutation with a global test mutex or run these tests serially to prevent cross-test interference. [race condition]
Severity Level: Major ⚠️
- ⚠️ Feature-flag env tests can race causing flakiness.
- ⚠️ Concurrent env mutation risks undefined behavior per Rust docs.Steps of Reproduction ✅
1. Run the workspace tests with concurrency enabled (default) using `cargo test` or `cargo
test -p phenotype-flags` so the Rust test harness can execute multiple `#[test]` functions
in parallel threads.
2. In `crates/phenotype-flags/src/lib.rs:300-308`, `test_from_env_invalid_value` mutates
the process environment via `unsafe { std::env::set_var("TESTAPP_BADFLAG", "maybe") };`
and later calls `FlagSet::from_env("TESTAPP")`.
3. In the same module, `test_from_env_skips_nonprefixed` at
`crates/phenotype-flags/src/lib.rs:310-320` and `test_from_env_skips_exact_prefix_match`
at `crates/phenotype-flags/src/lib.rs:326-339` also mutate the global environment using
`std::env::set_var` / `std::env::remove_var` inside `unsafe` blocks (lines 312-315 and
331-333, 335-338) without any cross-test synchronization.
4. Because the test harness may schedule these tests concurrently, and other tests in the
workspace also call `std::env::set_var` (e.g.,
`phenotype-infrakit/crates/phenotype-sentry-config/src/lib.rs:102` and
`agileplus/crates/agileplus-domain/src/config/tests.rs:45-47`), environment reads in
`FlagSet::from_env` at `crates/phenotype-flags/src/lib.rs:154-180` can race with global
env mutation, violating Rust's requirement that env modification not be performed
concurrently and leading to undefined behavior or flaky, nondeterministic test outcomes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/phenotype-flags/src/lib.rs
**Line:** 312:320
**Comment:**
*Race Condition: These tests mutate global process environment variables without any serialization, so parallel test execution can race across threads and produce flaky failures (or undefined behavior under Rust's unsafe env mutation contract). Guard env mutation with a global test mutex or run these tests serially to prevent cross-test interference.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![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: b180875bf5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| use super::*; | ||
| use std::collections::BTreeMap; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
AGENTS.md for this repo requires every test to include a // Traces to: FR-XXX-NNN comment, but the new phenotype-flags test module starts adding #[test]s without any traceability comments. Since this crate is also added as a workspace member, the documented traceability scan will flag these tests until each new test is tied to an FR.
Useful? React with 👍 / 👎.
|
|
||
| #[test] | ||
| fn test_from_env_empty_prefix_key_is_error() { | ||
| unsafe { std::env::set_var("MYAPP_", "true") }; |
There was a problem hiding this comment.
When Rust runs cargo test with the default parallel test harness, this MYAPP_ variable is process-global and can be visible to the other new tests that simultaneously call FlagSet::from_env("MYAPP"); in that overlap they will see this empty-key variable and return InvalidValue instead of the expected successful flag set. Use unique prefixes per test or serialize/guard the environment mutations so the workspace test run is not flaky.
Useful? React with 👍 / 👎.
1 participant
User description
Adds ADR cross-reference table to AGENTS.md as mandated by T12-B + T12-J. Maps all architecture decision records (ADR-001 through ADR-014 in ADR.md, plus docs/adr/ files) for agent discoverability.
CodeAnt-AI Description
Add a reusable feature-flag crate and update repo-wide docs and workspace setup
What Changed
phenotype-flagscrate for in-memory feature flags with environment-variable loading and sorted snapshotsAGENTS.mdwith an ADR cross-reference table and added coverage commands to the project helper fileImpact
✅ Faster flag checks✅ Clearer feature-flag loading from environment✅ Easier crate discovery for agents and maintainers💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.