Added ADR for architectural standards#2358
Conversation
Reviewer's GuideAdds ADR-00017 describing the introduction, purpose, and lifecycle of a repository-level CONVENTIONS.md file and documenting a detailed catalog of existing performance and coding anti-patterns in the Trustify codebase, along with convention options the team should select during review. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 1 issue, and left some high level feedback:
- The ADR is very long and mixes the high-level decision (introducing CONVENTIONS.md) with a detailed audit of anti-patterns; consider splitting the anti-pattern catalog into a separate document (or appendix) to keep the ADR focused on the architectural decision and process.
- Many examples reference approximate line numbers and specific file locations that will quickly become stale; instead of line-based pointers, consider linking to symbols or providing higher-level file/section references that will age more gracefully.
- You define option sets (A/B/C) for many categories but the process for capturing the final choice in CONVENTIONS.md is only described informally; consider adding a concise, explicit section on how and where the selected options are recorded and how future ADRs should evolve or override them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ADR is very long and mixes the high-level decision (introducing CONVENTIONS.md) with a detailed audit of anti-patterns; consider splitting the anti-pattern catalog into a separate document (or appendix) to keep the ADR focused on the architectural decision and process.
- Many examples reference approximate line numbers and specific file locations that will quickly become stale; instead of line-based pointers, consider linking to symbols or providing higher-level file/section references that will age more gracefully.
- You define option sets (A/B/C) for many categories but the process for capturing the final choice in CONVENTIONS.md is only described informally; consider adding a concise, explicit section on how and where the selected options are recorded and how future ADRs should evolve or override them.
## Individual Comments
### Comment 1
<location path="docs/adrs/00017-conventions-file.md" line_range="107" />
<code_context>
+
+## Anti-Pattern Analysis
+
+Analysis of the trustify codebase has identified recurring anti-patterns across
+multiple modules. This section catalogs each category, lists the occurrences found, and
+presents convention options for the team to evaluate. The selected conventions will be
</code_context>
<issue_to_address>
**nitpick (typo):** Capitalize the project name "Trustify" for consistency.
This line uses lowercase "trustify" while earlier references use "Trustify"; update this occurrence to match the proper noun capitalization.
```suggestion
Analysis of the Trustify codebase has identified recurring anti-patterns across
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mrrajan
force-pushed
the
TC-4289
branch
2 times, most recently
from
156ce43 to
79bfc3b
Compare
|
Thanks @mrrajan Very interesting reading I'd like us to come up with a way of prioritizing our next steps. As I would like to think there is some non contentious conventions we can adopt that will address some of these things. And we might need some Spikes to evaluate the best convention for some of the issues highlighted. |
mrizzi
left a comment
mrizzi
left a comment
There was a problem hiding this comment.
A first review of the methodology before getting into the anti-patterns analysis with the maintainers.
| ### Content Principles | ||
|
|
||
| 1. **Prescriptive, not descriptive**: each convention states what to do and what to avoid, with concrete code examples | ||
| 2. **Derived from existing code**: conventions are extracted from established patterns in the codebase, not invented |
There was a problem hiding this comment.
Maybe the initial version but not once stable because, once fully adopted, it should be other way around, i.e. code should be derived from existing conventions
There was a problem hiding this comment.
Question is, how do we deal with current code that doesn't align to those newly defined patterns?
There was a problem hiding this comment.
Good question 👍
Sending random and sometimes partial PRs without JIRAs?
side note: Yesterday I asked AI to read the instrumentation section and to produce a checklist for me, not changed the code yet as some parts are subjective (example -> the size of returned struct data) + still need to validate if all the checklist is 100% correct. Imagining all this checklist is correct, all the instrumentation-related-code from all files should be updated via a single PR? Should this initiative be separated by our current workspace-crates to avoid git conflicts and long PR reviews?
There was a problem hiding this comment.
Question is, how do we deal with current code that doesn't align to those newly defined patterns?
It will mean having tech debt initiatives to enforce consistency in current code not yet aligned with expected patterns and code conventions.
|
|
||
| ### Trade-offs | ||
|
|
||
| - **Maintenance burden**: the file must be kept in sync with evolving practices — stale conventions are worse than no conventions |
There was a problem hiding this comment.
I don't think this exists because, as described in the above Maintenance Process, when a convention has to change, then an ADR arrives. Once merged, the impacted code has to refactored to be aligned with the new code convention. I don't see how "stale conventions" could happen other than implementing code not aligned with the conventions, which should be considered a mistake to prevent.
There was a problem hiding this comment.
To my understanding, feedback from reviews might turn in PR changing the conventions file? If that's the case, then this scenario may happen. Unless of course, a change in the conventions file is accompanied by reworking the code base.
There was a problem hiding this comment.
Changes from PRs are meant to be additive to improve code conventions for identified lacks. Changing them to "adhere" to the code in a PR should be blocked otherwise code will drift from conventions.
| - **Occurrences found** — specific locations in the codebase | ||
| - **Convention options** — choices for the team to decide on (to be resolved via PR review) | ||
|
|
||
| > **How to use this section**: During PR review, maintainers should mark their preferred |
There was a problem hiding this comment.
@jcrossley3 @ctron @ruromero @queria please review this
|
I'm not sure an ADR is the right approach to this. We already have the conventions file, and this ADR would need to be copied over to that file in a second step. Why no simply make the modifications to the conventions file and discuss this on a PR? Because what would we review on that second PR (moving content over to the conventions file)? Feels like doppelmoppel. |
|
|
||
| ## Context | ||
|
|
||
| Trustify uses AI-assisted development workflows (Claude Code, Copilot, and similar tools). These tools perform best when they have access to explicit, machine-readable project conventions — coding patterns, naming rules, error-handling idioms, testing practices, and architectural norms. |
There was a problem hiding this comment.
As we do have some documentation (for less then we should have) for humans. I believe that this file should be usable by humans as well. Replacing the existing documents. Which means, that this file should be structured and written to serve both.
There was a problem hiding this comment.
Agree! Rephrased to,
Trustify needs a single, explicit reference for coding patterns, naming rules, error-handling idioms, testing practices, and architectural norms. Contributors and reviewers use it during implementation and review; the same clarity also helps AI-assisted workflows (Claude Code, Copilot, and similar tools) when those tools load project context.
| | **Entity Model Patterns** | ORM model conventions | `DeriveEntityModel`, relations, `Linked` structs | | ||
| | **Migration Patterns** | Database migration conventions | Idempotency guards, naming, raw SQL loading | | ||
| | **Rust Idioms** | Preferred Rust patterns | Type inference, iterator ownership, `.zip()`, capacity | | ||
| | **SeaORM Query Patterns** | ORM query conventions | `.is_in()`, chunking, `IntoIterator` parameters | |
There was a problem hiding this comment.
IntoIterator wouldn't be a SeaORM thing, but a Rust thing.
| | **Rust Idioms** | Preferred Rust patterns | Type inference, iterator ownership, `.zip()`, capacity | | ||
| | **SeaORM Query Patterns** | ORM query conventions | `.is_in()`, chunking, `IntoIterator` parameters | | ||
| | **Observability** | Tracing and instrumentation | `#[instrument]` usage, span conventions, error levels | | ||
| | **Shared Table Patterns** | Concurrent insert handling | Nested transaction duplicate-key and `.on_conflict` patterns | |
There was a problem hiding this comment.
Not sure I understand the scope of this section. I'd prefer to have some "database" section maybe. Defining how database resources/entities are created, column types, tables, enums, indexes, ...
There was a problem hiding this comment.
Agree! We should have a dedicated database section.
| 1. **Prescriptive, not descriptive**: each convention states what to do and what to avoid, with concrete code examples | ||
| 2. **Derived from existing code**: conventions are extracted from established patterns in the codebase, not invented | ||
| 3. **Minimal and actionable**: each entry should be short enough that a contributor (or AI tool) can apply it without reading surrounding prose | ||
| 4. **Reference implementations**: point to specific files in the codebase as canonical examples |
There was a problem hiding this comment.
I think that may be tricky. As those code locations will change over time. And that would mean updating the conventions file, or dead links.
There was a problem hiding this comment.
Agree! We should have just the examples not the references to the specific files. Rephrased to share canonical examples on the conventions file.
| - Claude Code automatically loads `CONVENTIONS.md` from the repository root as part of its project context | ||
| - The file uses markdown with code blocks, making it parseable by any LLM | ||
| - Conventions are structured as clear rules with examples, optimizing for AI instruction-following | ||
| - When a `CLAUDE.md` file is present (for tool-specific configuration), `CONVENTIONS.md` complements it — `CONVENTIONS.md` focuses on language and framework patterns that apply regardless of the tool |
There was a problem hiding this comment.
I wonder how that aligns with review tools? like sourcery.ai?
There was a problem hiding this comment.
Sourcery.ai applies generic best practices and CONVENTIONS.md captures project specific ones. So, project specific practices might called out by the review tools. We have to explore how to align review tools to use CONVENTIONS.md file as reference.
| ## Anti-Pattern Analysis | ||
|
|
||
| Analysis of the Trustify codebase has identified recurring anti-patterns across | ||
| multiple modules. This section catalogs each category, lists the occurrences found, and |
There was a problem hiding this comment.
I see the same problem as before. If we "point" to those locations, and those locations will be gone over time (which should be the case with anti-patterns) we have a document of dead links.
There was a problem hiding this comment.
Agree! The references should use function/ symbol names or example snippets in the CONVENTIONS.md file
|
|
||
| Each category below follows this structure: | ||
| - **What it is** — description of the anti-pattern | ||
| - **Why it matters** — performance impact |
There was a problem hiding this comment.
I assume "performance impact" is an example. But in general, "why" can be all kind of reasons.
There was a problem hiding this comment.
Agree! rephrased to performance impact, maintainability, consistency
| Each category below follows this structure: | ||
| - **What it is** — description of the anti-pattern | ||
| - **Why it matters** — performance impact | ||
| - **Occurrences found** — specific locations in the codebase |
There was a problem hiding this comment.
See my comment about links above.
There was a problem hiding this comment.
Agree! This should be references not specific locations in codebase.
| **What it is**: Database queries executed inside loops — fetching related data one entity | ||
| at a time instead of loading all related data in a single batch query. | ||
|
|
||
| **Why it matters**: For a list of N entities, this generates N additional queries instead of |
There was a problem hiding this comment.
On the "pro" side, it may reduce code complexity. In some cases dramatically. And when it is known that "N" will always be a small number, and the operation itself is fast. Then keeping the N+1 pattern would be a good thing IMHO. It depends on the case.
| 1 batch query. On API read paths serving collections, this scales linearly with result size | ||
| and dominates response latency. | ||
|
|
||
| **Occurrences found (5)**: |
There was a problem hiding this comment.
I think there are a lot more in, especially in the "analysis" module.
|
|
||
| **Occurrences found (5)**: | ||
|
|
||
| | # | File | Line(s) | Description | Severity | |
There was a problem hiding this comment.
How is "severity" evaluated here?
There was a problem hiding this comment.
Severity is based on,
- Whether the N is bounded or not
- Code path is API or internal
|
|
||
| **Convention options**: | ||
|
|
||
| - **Option A — Batch with JOINs/IN clauses**: All collection data access MUST use batch |
There was a problem hiding this comment.
Maybe we should consider this: https://pganalyze.com/blog/5mins-postgres-performance-in-vs-any
| - **Option C — Keep as-is**: No convention. Performance is addressed case-by-case when | ||
| bottlenecks are observed. N+1 patterns are acceptable if the loop iteration count is | ||
| expected to be small. | ||
|
|
There was a problem hiding this comment.
I would say "Option D": prefer option A, but fall back to "N+1" if there are good reasons.
"C" doesn't seem to cover this, as it says "no convention". I think the should try, and try hard to remediate N+1 patterns. But not at all cost.
There was a problem hiding this comment.
Sounds good! Added option D
- **Option D — Prefer batch, allow exceptions**: Default to Option A — batch loading on
collection paths and remediate existing N+1 patterns as tech debt. Per-entity DB calls
inside loops are permitted only when batching is impractical or costly.
|
|
||
| | # | File | Line(s) | Description | | ||
| |---|------|---------|-------------| | ||
| | 1 | `modules/importer/src/service.rs` | ~133 | `importer::Entity::find().all()` fetches all importers, sorts in memory | |
There was a problem hiding this comment.
Which I don't see as a problem, as the number of importers is considered small (below 50). Sure it may be an improvement. But in some cases it's not. Having pagination on the HTTP API would be beneficial.
| **Why it matters**: As data grows, unbounded queries consume increasing memory and network | ||
| bandwidth. A single unbounded query on a large table can cause OOM or timeout. | ||
|
|
||
| **Occurrences found (3)**: |
There was a problem hiding this comment.
Again, there is more. Especially in the analysis module.
|
|
||
| ### AP-2: Unbounded Queries | ||
|
|
||
| **What it is**: Queries that fetch all rows from a table or relation without applying |
There was a problem hiding this comment.
In some cases we load all data, and then keep in memory to process it. Especially as mentioned above to do things like batch loading, pre-loading, ...
In such cases it doesn't seem to make sense to use limits, as we load and store the data in-memory anyway. In fact, it would be counter productive (even batch loading). Batch loading only makes sense to limit the number of postgres parameters. Having to page through data would mean to execute the query multiple times. Compared to one single query.
There was a problem hiding this comment.
Agree! lets limit only for the public API endpoints
| must use `.limit()` or chunked iteration. Exceptions only for tables with a known small | ||
| upper bound (e.g., importers, which are admin-configured). | ||
|
|
||
| - **Option B — Limit only on public API endpoints**: Public API list endpoints must be |
|
|
||
| | # | File | Line(s) | Description | | ||
| |---|------|---------|-------------| | ||
| | 1 | `modules/fundamental/src/vulnerability/model/details/mod.rs` | ~54-68 | All scores fetched via `.all(tx)`, then filtered with `.filter(\|s\| s.advisory_id == ...)` | |
There was a problem hiding this comment.
I'd say that's a false positive. As the code does re-use that data. But eliminates a second query by re-using that data and manually filtering out information for the first use case of the data.
There was a problem hiding this comment.
Agree! It is a false positive. I will remove this example section - But as a convention for this issue, shall we suggest option B?
|
|
||
| | # | File | Line(s) | Pattern | Severity | | ||
| |---|------|---------|---------|----------| | ||
| | 1 | `modules/importer/src/model/mod.rs` | ~331 | `serde_json::from_value(report).ok()` — importer run report deserialization silently dropped | High | |
There was a problem hiding this comment.
In this case, this provides additional diagnostic information, if possible. If the serialization of that fails, that would just not be possible. Logging would be fine, but throwing an error not.
There was a problem hiding this comment.
Agree! It is additional noise
| | # | File | Line(s) | Pattern | Severity | | ||
| |---|------|---------|---------|----------| | ||
| | 1 | `modules/importer/src/model/mod.rs` | ~331 | `serde_json::from_value(report).ok()` — importer run report deserialization silently dropped | High | | ||
| | 2 | `modules/importer/src/server/progress.rs` | ~48, 77 | `let _ = self.service.set_progress_message(...)` — DB write failures for progress silently discarded | High | |
There was a problem hiding this comment.
In this case it would actually be beneficial to add this to the instrumentation. Rather than an explicit log call.
| | 1 | `modules/importer/src/model/mod.rs` | ~331 | `serde_json::from_value(report).ok()` — importer run report deserialization silently dropped | High | | ||
| | 2 | `modules/importer/src/server/progress.rs` | ~48, 77 | `let _ = self.service.set_progress_message(...)` — DB write failures for progress silently discarded | High | | ||
| | 3 | `modules/importer/src/server/mod.rs` | ~186 | `serde_json::to_value(report).ok()` — importer run report serialization silently dropped on `update_finish`; report data lost from database | High | | ||
| | 4 | `modules/importer/src/model/mod.rs` | ~65 | `OffsetDateTime::from_unix_timestamp_nanos(t).ok()` — heartbeat timestamp errors silently dropped | Medium | |
There was a problem hiding this comment.
Here it might actually be beneficial to remove the root cause of this. Using a proper timestamp in the database, if possible.
There was a problem hiding this comment.
Agree! we should fix the root cause instead!
| | 2 | `modules/importer/src/server/progress.rs` | ~48, 77 | `let _ = self.service.set_progress_message(...)` — DB write failures for progress silently discarded | High | | ||
| | 3 | `modules/importer/src/server/mod.rs` | ~186 | `serde_json::to_value(report).ok()` — importer run report serialization silently dropped on `update_finish`; report data lost from database | High | | ||
| | 4 | `modules/importer/src/model/mod.rs` | ~65 | `OffsetDateTime::from_unix_timestamp_nanos(t).ok()` — heartbeat timestamp errors silently dropped | Medium | | ||
| | 5 | `modules/ingestor/src/service/advisory/csaf/loader.rs` | ~44, 48 | `published`/`modified` timestamp conversions silently become `None` | Medium | |
There was a problem hiding this comment.
In this case it would be beneficial to report this back to the user, as we do have a mechanism for reporting such things as "warning" during the upload. This would also end up in the report. Logging this, or failing, wouldn't help.
There was a problem hiding this comment.
Same for the other CSAF related ones.
|
|
||
| - **Option C — Keep as-is**: No convention. Developers judge whether an error is worth | ||
| logging. `.ok()` is acceptable for non-critical conversions. | ||
|
|
There was a problem hiding this comment.
Option D:
- if we have instrumentation wrapping it, that's ok, no need for extra processing
- if we have a way to report this case back to the user, leverage this, but don't extra log it
- if that case prevents us from processing the data, and the data is essential, propagate the error
- if running into an error is an acceptable and expected case, that's fine, do nothing
- if we can ignore the error, but have no other way to handle it, log it, but try to be compact doing it, e.g. like
.inspect_err()rather than a full blownmatch.
|
|
||
| **Convention options**: | ||
|
|
||
| - **Option A — Enums for fixed value sets**: Any value drawn from a fixed set (statuses, |
|
|
||
| **Convention options**: | ||
|
|
||
| - **Option A — Extract shared logic into generics or traits**: When two or more modules |
| composition. However, the `fundamental` layer MUST NOT depend on `ingestor` — shared | ||
| types like `Deprecation` must be moved to `common/`. | ||
|
|
||
| - **Option C — Keep as-is**: No convention. Cross-module imports are acceptable within |
| phase into a private helper method with a descriptive name. The parent function becomes | ||
| a coordinator that calls the helpers in sequence. | ||
|
|
||
| - **Option B — Maximum cyclomatic complexity**: No hard line limit, but functions with |
There was a problem hiding this comment.
Option B. And I'd say it is ok to use inner strucs, fns, ... for doing this.
|
|
||
| - **Option C — Keep as-is**: No convention. Instrumentation is added when needed for | ||
| debugging. Partial coverage is acceptable. | ||
|
|
There was a problem hiding this comment.
Option D: Option A, but with a twist. I think this is already well described in the tracing/logging markdown file. In a nutshell, everything that is calling out to external things (DB, storage, ... ) should be wrapped in instrumentation. Everything that does not, as long as it is considered trivial, must not be wrapped. Even if things cannot be wrapped (multiple sections of loading where loading functions don't have instrumentation) use .instrument or other constructs amending this. But don't do it twice (inner vs outer). Not limited to pub, but based on logic blocks and the fact that external factors are in play.
|
|
||
| **Convention options**: | ||
|
|
||
| - **Option A — Document all public items**: Every public struct, enum, trait, function, |
There was a problem hiding this comment.
Option A.
In addition, also add non-verbose comments inside functions.
| and `tracing::instrument`/`tracing::Instrument` attributes, creating inconsistent | ||
| observability output. | ||
|
|
||
| **Why it matters**: `tracing` and `log` have different span-awareness behavior. `log::*` |
There was a problem hiding this comment.
Not sure this is true, as log actually forwards to tracing.
There was a problem hiding this comment.
Referring to https://docs.rs/tracing-log/latest/tracing_log/, we need logTracer::init() to have logs and tracing connected. But in our project only the test file, test-context/src/flame.rs has this configuration for setup_global_subscriber.
There was a problem hiding this comment.
we are using a tracing-log feature here: https://github.com/guacsec/trustify/blob/main/common/infrastructure/Cargo.toml#L33
log actually forwards to tracing
this explains why even using log::info here https://gist.github.com/helio-frota/171ac4ac8d362827e22b66ce165f32cf?permalink_comment_id=6121576#gistcomment-6121576, I was able to see the correlation with traces using opentelemetry-appender-tracing instead of opentelemetry-appender-log 👍
There was a problem hiding this comment.
Thanks for the clarification @helio-frota! updated this section.
|
|
||
| **Convention options**: | ||
|
|
||
| - **Option A — Use `tracing::` exclusively**: All logging MUST use `tracing::` macros |
|
|
||
| - **Option C — Keep as-is**: No convention. Inline values are acceptable with explanatory | ||
| comments. | ||
|
|
There was a problem hiding this comment.
Option D: It depends. In general we should have Option A. However, there are cases where it causes confusion or has other side effects.
Assume the example from above. Having 13 replaced by a const value, would require to render the string every time used. Rather than having a const struct. This seems counter productive. It would be better to keep that 13 in the string, but document around it "why" there's a 13 in there.
Same for structs:
ImporterConfiguration::Cve(CveImporter {
common: CommonImporter {
disabled: true,
period: Duration::from_secs(300),
description: Some(description.into()),
labels: Default::default(),
},
source: DEFAULT_SOURCE_CVEPROJECT.into(),
years: HashSet::default(),
start_year,
}),Having DEFAULT_SOURCE_CVEPROJECT here, imported from another module. Only being used once. Doesn't make much sense to me. It would be clearer if that would be in the same file. Unless, it would indeed be re-used. Then it could be refactored.
|
|
||
| | # | File | Line(s) | Description | | ||
| |---|------|---------|-------------| | ||
| | 1 | `modules/fundamental/src/vulnerability/service/mod.rs` | ~455-657 | `build_vulnerabilities_query_string` (~455-521) uses `format!()` to interpolate SQL fragments; `build_query` (~523-657) constructs sub-queries with `Statement::from_sql_and_values`, then `.to_string()`s them and concatenates with `UNION ALL` into a `Statement::from_string` | |
There was a problem hiding this comment.
This example might not be ideal. But it seems like a decent trade off handling the complexity. Having that replaced by something else, the question would be if the alternative is actually better,
There is no SQL injection in play here, as that's gets sorted out before.
| SeaORM's `UnionType` builder or a single parameterized CTE. `Statement::from_string` | ||
| is only acceptable for static SQL with no dynamic values. | ||
|
|
||
| - **Option B — Allow with review gate**: `Statement::from_string` with dynamic content |
There was a problem hiding this comment.
Option B: There should be a good reason to not use Option A, and a discussion around that. But to me the example shows a valid reason to deviate from the norm.
| | **Entity Model Patterns** | ORM model conventions | `DeriveEntityModel`, relations, `Linked` structs | | ||
| | **Migration Patterns** | Database migration conventions | Idempotency guards, naming, raw SQL loading | | ||
| | **Rust Idioms** | Preferred Rust patterns | Type inference, iterator ownership, `.zip()`, capacity | | ||
| | **SeaORM Query Patterns** | ORM query conventions | `.is_in()`, chunking, `IntoIterator` parameters | |
| | **Rust Idioms** | Preferred Rust patterns | Type inference, iterator ownership, `.zip()`, capacity | | ||
| | **SeaORM Query Patterns** | ORM query conventions | `.is_in()`, chunking, `IntoIterator` parameters | | ||
| | **Observability** | Tracing and instrumentation | `#[instrument]` usage, span conventions, error levels | | ||
| | **Shared Table Patterns** | Concurrent insert handling | Nested transaction duplicate-key and `.on_conflict` patterns | |
There was a problem hiding this comment.
Agree! We should have a dedicated database section.
| 1. **Prescriptive, not descriptive**: each convention states what to do and what to avoid, with concrete code examples | ||
| 2. **Derived from existing code**: conventions are extracted from established patterns in the codebase, not invented | ||
| 3. **Minimal and actionable**: each entry should be short enough that a contributor (or AI tool) can apply it without reading surrounding prose | ||
| 4. **Reference implementations**: point to specific files in the codebase as canonical examples |
There was a problem hiding this comment.
Agree! We should have just the examples not the references to the specific files. Rephrased to share canonical examples on the conventions file.
| - Claude Code automatically loads `CONVENTIONS.md` from the repository root as part of its project context | ||
| - The file uses markdown with code blocks, making it parseable by any LLM | ||
| - Conventions are structured as clear rules with examples, optimizing for AI instruction-following | ||
| - When a `CLAUDE.md` file is present (for tool-specific configuration), `CONVENTIONS.md` complements it — `CONVENTIONS.md` focuses on language and framework patterns that apply regardless of the tool |
There was a problem hiding this comment.
Sourcery.ai applies generic best practices and CONVENTIONS.md captures project specific ones. So, project specific practices might called out by the review tools. We have to explore how to align review tools to use CONVENTIONS.md file as reference.
| SeaORM's `UnionType` builder or a single parameterized CTE. `Statement::from_string` | ||
| is only acceptable for static SQL with no dynamic values. | ||
|
|
||
| - **Option B — Allow with review gate**: `Statement::from_string` with dynamic content |
|
|
||
| ## Context | ||
|
|
||
| Trustify uses AI-assisted development workflows (Claude Code, Copilot, and similar tools). These tools perform best when they have access to explicit, machine-readable project conventions — coding patterns, naming rules, error-handling idioms, testing practices, and architectural norms. |
There was a problem hiding this comment.
Agree! Rephrased to,
Trustify needs a single, explicit reference for coding patterns, naming rules, error-handling idioms, testing practices, and architectural norms. Contributors and reviewers use it during implementation and review; the same clarity also helps AI-assisted workflows (Claude Code, Copilot, and similar tools) when those tools load project context.
|
|
||
| ### AP-2: Unbounded Queries | ||
|
|
||
| **What it is**: Queries that fetch all rows from a table or relation without applying |
There was a problem hiding this comment.
Agree! lets limit only for the public API endpoints
| | 1 | `modules/fundamental/src/vulnerability/model/details/mod.rs` | ~54-68 | All scores fetched via `.all(tx)`, then filtered with `.filter(\|s\| s.advisory_id == ...)` | | ||
| | 2 | `modules/fundamental/src/advisory/model/summary.rs` | ~40-67 | Bulk scores fetched, then O(advisories x vulns x scores) in-memory iteration | | ||
|
|
||
| **Convention options**: |
| and `tracing::instrument`/`tracing::Instrument` attributes, creating inconsistent | ||
| observability output. | ||
|
|
||
| **Why it matters**: `tracing` and `log` have different span-awareness behavior. `log::*` |
There was a problem hiding this comment.
Referring to https://docs.rs/tracing-log/latest/tracing_log/, we need logTracer::init() to have logs and tracing connected. But in our project only the test file, test-context/src/flame.rs has this configuration for setup_global_subscriber.
mrrajan
force-pushed
the
TC-4289
branch
2 times, most recently
from
00239e8 to
9c838c3
Compare
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
…ntions Signed-off-by: mrrajan <86094767+mrrajan@users.noreply.github.com.>
| - **Option A — Extract shared logic into generics or traits**: When two or more modules | ||
| implement the same logic pattern (e.g., label CRUD, license filtering), extract it into | ||
| a shared generic function, trait, or macro parameterized by the entity type. Duplicated | ||
| logic blocks longer than ~20 lines are prohibited. |
There was a problem hiding this comment.
informational only:
I quickly modified my personal duplicate code segment tool to focus on 20+ lines and found only these: https://gist.github.com/helio-frota/87f1a9ab12fe84480f2574bc7b92e8a7
( I'm skipping tests,test,entity,migration,benches )
ctron
left a comment
ctron
left a comment
There was a problem hiding this comment.
Looks good to me. With the exception that I still see the actual content (AP, …) My understanding was, that his would be removed from this ADR, but then added in another PR to the conventions file.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 participants
Summary
Rendered version https://github.com/mrrajan/trustify/blob/TC-4289/docs/adrs/00018-conventions-file.md
Summary by Sourcery
Introduce an ADR proposing a centralized CONVENTIONS.md file and cataloging key performance and coding anti‑patterns in the Trustify codebase, along with candidate standards for addressing them.
Documentation: