Skip to content

Add comprehensive architecture improvement backlog#3207

Open
robnester-rh wants to merge 1 commit intoconforma:mainfrom
robnester-rh:ai_improvement_suggestions
Open

Add comprehensive architecture improvement backlog#3207
robnester-rh wants to merge 1 commit intoconforma:mainfrom
robnester-rh:ai_improvement_suggestions

Conversation

@robnester-rh
Copy link
Copy Markdown
Contributor

Summary

  • Adds docs/ARCHITECTURE_BACKLOG.md documenting identified architectural issues, technical debt, and improvement opportunities
  • Based on deep-dive analysis of the codebase using parallel exploration agents
  • Provides prioritized roadmap for remediation

Key Findings

Category Count Severity
Critical Logic/Correctness Bugs 8 High
Incomplete/Stub Code 5 Medium-High
Silent Error Handling 12 Medium-High
API/Naming Inconsistencies 15 Medium
Hardcoded Values 25+ Medium
File Size Issues (>500 LOC) 24 Medium

Highlights

  • P0 Issues: EffectiveTime() value receiver bug, worker count off-by-one, result duplication in filters
  • Silent Failures: UploadVSAEnvelope always returns nil, prepareDataDirs ignores walk errors
  • Architecture: conftest_evaluator.go mixes 10+ responsibilities, two parallel pre-filtering mechanisms

Test Plan

  • Review document for accuracy
  • Prioritize items for future work
  • No code changes - documentation only

Ref: EC-9999

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Fix version bump and add comprehensive architecture backlog

📝 Documentation 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fixes bump-minor-version Makefile target to handle version 0.9 correctly using awk instead of
  floating-point arithmetic
• Adds comprehensive 834-line architecture improvement backlog documenting identified issues and
  technical debt
• Catalogs 8 critical logic bugs, 12 silent error handlers, 24 large files, and 15+ API
  inconsistencies
• Provides prioritized roadmap with 6 implementation phases for remediation
Diagram
flowchart LR
  A["Makefile bump-minor-version"] -->|Replace yq floating-point| B["awk string-based version increment"]
  C["Codebase Analysis"] -->|Identify issues| D["Architecture Backlog Document"]
  D -->|Categorize| E["8 Critical Bugs"]
  D -->|Categorize| F["12 Silent Errors"]
  D -->|Categorize| G["24 Large Files"]
  D -->|Categorize| H["15+ API Issues"]
  D -->|Provide| I["6-Phase Roadmap"]
Loading

Grey Divider

File Changes

1. Makefile 🐞 Bug fix +3/-2

Fix version bump arithmetic for 0.x versions

• Replaces yq floating-point arithmetic with awk-based string splitting for version increment
• Fixes off-by-one issue where version 0.9 incorrectly became 1.0 instead of 0.10
• Preserves major.minor format across all version numbers
• Updates commit message to use computed version variable

Makefile


2. docs/ARCHITECTURE_BACKLOG.md 📝 Documentation +834/-0

Comprehensive architecture backlog with prioritized issues

• New 834-line comprehensive architecture improvement backlog document
• Documents 8 critical logic/correctness bugs with P0 priority (EffectiveTime receiver, worker count
 off-by-one, result duplication)
• Catalogs 12 silent error handling issues (prepareDataDirs, UploadVSAEnvelope, ProcessAllVSAs)
• Lists 5 incomplete/stub code items, 15+ API/naming inconsistencies, 25+ hardcoded values
• Identifies 24 source files over 500 lines and 9 test files over 1000 lines for refactoring
• Describes 7 major architecture issues including conftest_evaluator overloading and parallel filter
 mechanisms
• Provides 6-phase implementation roadmap with effort estimates and success metrics

docs/ARCHITECTURE_BACKLOG.md


Grey Divider

Qodo Logo

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 03d03760-b7c1-48ef-aac2-de5025eca15e

📥 Commits

Reviewing files that changed from the base of the PR and between 7e02e24 and 9fd6301.

📒 Files selected for processing (1)
  • docs/ARCHITECTURE_BACKLOG.md
✅ Files skipped from review due to trivial changes (1)
  • docs/ARCHITECTURE_BACKLOG.md

📝 Walkthrough

Walkthrough

Adds a comprehensive architecture backlog documentation file that catalogs identified improvements for the Conforma CLI codebase, including critical bugs, incomplete features, error-handling gaps, API mismatches, hardcoded values, architectural concerns, and a phased implementation roadmap. No runtime code is modified.

Changes

Cohort / File(s) Summary
Architecture Documentation
docs/ARCHITECTURE_BACKLOG.md
New documentation file providing an executive summary, categorized issue counts with severity/priority legends, and enumerated backlog items spanning critical logic bugs, incomplete functionality, silent error handling, API mismatches, hardcoded values, architectural concerns, global state issues, and a phased improvement roadmap.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add comprehensive architecture improvement backlog' directly and clearly describes the main change: addition of a new architecture backlog documentation file.
Description check ✅ Passed The description is well-related to the changeset, providing summary, key findings, highlights, and test plan for the architecture backlog documentation being added.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Mar 27, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Remediation recommended

1. VERSION format not validated 🐞 Bug ✓ Correctness
Description
bump-minor-version increments the minor component via awk without validating that VERSION is
strictly numeric major.minor, so malformed values (e.g., 0.9.1 or v0.9) can be rewritten into
an incorrect version without failing. This can break release/version derivation because
hack/derive-version.sh assumes VERSION contains only the major.minor string.
Code

Makefile[R379-380]

+	@NEW_VERSION=$$(awk -F. '{printf "%s.%d", $$1, $$2+1}' $(VERSION_FILE)) && \
+	  echo "$$NEW_VERSION" > $(VERSION_FILE) && \
Evidence
The Makefile now derives NEW_VERSION by splitting on . and incrementing only the second field,
with no checks on field count or numeric content. The release version derivation script explicitly
documents and relies on VERSION containing only major.minor, and concatenates it directly into
the produced semver string; silently rewriting an unexpected format can therefore produce incorrect
release versions.

Makefile[379-384]
hack/derive-version.sh[22-25]
hack/derive-version.sh[35-66]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`make bump-minor-version` increments the minor version using `awk` but does not validate that `VERSION` contains exactly two numeric dot-separated fields (`major.minor`). This can silently produce incorrect versions if the file format ever deviates (e.g., `0.9.1` becomes `0.10`, `v0.9` produces a malformed result).

## Issue Context
`hack/derive-version.sh` assumes `VERSION` contains the `major.minor` string and embeds it directly into the output version. Failing fast on an unexpected VERSION format prevents incorrect release artifacts.

## Fix Focus Areas
- Makefile[379-384]
- hack/derive-version.sh[22-25]

## Suggested fix
Add a format check before computing `NEW_VERSION`, e.g.:
- Read the current value and assert `^[0-9]+\.[0-9]+$`.
- If it fails, print a clear error and exit non-zero.

(Optionally) implement the bump using `awk` with validation, e.g. `awk -F. 'NF!=2 || $1!~/^[0-9]+$/ || $2!~/^[0-9]+$/ {exit 2} {printf "%s.%d\n", $1, $2+1}'` and propagate the non-zero exit.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/ARCHITECTURE_BACKLOG.md`:
- Around line 495-519: The "### Source Files Over 500 Lines" section contains
entries under the 500-line threshold causing inconsistent metrics; update the
docs by either renaming the section header (e.g., change "### Source Files Over
500 Lines" to "Source Files Over N Lines" or "Source Files Over 350 Lines") or
move files with lines <500 into a separate table/bucket (e.g., "Files Under 500
Lines") and leave only files >=500 in the original table; locate the section by
the header string "### Source Files Over 500 Lines" and the listed filenames
(e.g., cmd/validate/vsa.go, internal/applicationsnapshot/report.go,
internal/rego/sigstore/sigstore.go) and apply one of the two fixes to make the
heading and entries consistent.
- Around line 454-485: Add blank lines before and after each Markdown table to
satisfy markdownlint MD058; specifically update the tables under headings like
"5.2 URLs and Defaults", "5.3 Worker/Concurrency Defaults", "5.4 File
Permissions" and the earlier table that lists timeouts/values so that each table
is separated by an empty line above and below. Ensure you also apply the same
fix to the other repeated table occurrences noted in the review (the additional
table blocks referenced) so every table in the document has a blank line before
and after it.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 165dd366-b1ad-4bcf-92b1-6507a6305ee9

📥 Commits

Reviewing files that changed from the base of the PR and between 671c672 and f6d5fd1.

📒 Files selected for processing (2)
  • Makefile
  • docs/ARCHITECTURE_BACKLOG.md

@robnester-rh
Copy link
Copy Markdown
Contributor Author

Jira: EC-1726

@robnester-rh robnester-rh force-pushed the ai_improvement_suggestions branch from f6d5fd1 to 4003c0c Compare March 27, 2026 15:22
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/ARCHITECTURE_BACKLOG.md (1)

495-519: Section threshold and data are inconsistent, making backlog metrics unreliable.

Line 495 says “Source Files Over 500 Lines”, but multiple rows (e.g., Line 508–Line 519) are under 500. That inconsistency also undermines the metric in Line 818 (“Files > 500 lines | 24”). Please either keep only >=500 entries in this table or rename/split the section so counts and thresholds match.

As per coding guidelines "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

Also applies to: 815-819

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ARCHITECTURE_BACKLOG.md` around lines 495 - 519, The "Source Files Over
500 Lines" table header and the "Files > 500 lines | 24" metric are inconsistent
with entries that are under 500 lines; update the table generation so it only
lists files with lines >= 500 (or if you intended a different threshold, rename
the header to match the actual cutoff and adjust the entries accordingly) and
ensure the summary metric string ("Files > 500 lines | 24") is recalculated to
match the filtered list; locate and fix the table and metric strings around the
"Source Files Over 500 Lines" section and the "Files > 500 lines | 24" metric to
keep the threshold, rows and counts consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@docs/ARCHITECTURE_BACKLOG.md`:
- Around line 495-519: The "Source Files Over 500 Lines" table header and the
"Files > 500 lines | 24" metric are inconsistent with entries that are under 500
lines; update the table generation so it only lists files with lines >= 500 (or
if you intended a different threshold, rename the header to match the actual
cutoff and adjust the entries accordingly) and ensure the summary metric string
("Files > 500 lines | 24") is recalculated to match the filtered list; locate
and fix the table and metric strings around the "Source Files Over 500 Lines"
section and the "Files > 500 lines | 24" metric to keep the threshold, rows and
counts consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6e2a9b26-a425-40eb-b72b-9667a93ab69a

📥 Commits

Reviewing files that changed from the base of the PR and between f6d5fd1 and 4003c0c.

📒 Files selected for processing (1)
  • docs/ARCHITECTURE_BACKLOG.md

@robnester-rh robnester-rh force-pushed the ai_improvement_suggestions branch from 4003c0c to 7e02e24 Compare March 30, 2026 20:19
Document catalogs identified architectural issues, technical debt, and
improvement opportunities based on deep-dive analysis of the codebase.
Includes critical logic bugs, silent error handling issues, API
inconsistencies, hardcoded values, and large file candidates for
refactoring.

Key findings:
- 8 critical logic/correctness issues identified
- 12 silent error handling problems
- 24 source files over 500 lines
- Prioritized roadmap for remediation

Ref: EC-1726

Signed-off-by: Rob Nester <rnester@redhat.com>
Assisted-by: Cursor
Made-with: Cursor
@robnester-rh robnester-rh force-pushed the ai_improvement_suggestions branch from 7e02e24 to 9fd6301 Compare March 30, 2026 20:48
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
acceptance 55.17% <ø> (ø)
generative 17.90% <ø> (ø)
integration 26.63% <ø> (ø)
unit 69.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@robnester-rh
Copy link
Copy Markdown
Contributor Author

/retest

Copy link
Copy Markdown
Contributor

@st3penta st3penta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about turning this doc into something different? I don't really like the idea of having it committed in the repo.
Some alternative ideas are: a jira epic, an issue, a discussion, or a wiki.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants