Add comprehensive architecture improvement backlog#3207
Open
robnester-rh wants to merge 2 commits intoconforma:mainfrom
Open
Add comprehensive architecture improvement backlog#3207robnester-rh wants to merge 2 commits intoconforma:mainfrom
robnester-rh wants to merge 2 commits intoconforma:mainfrom
Conversation
The previous yq expression used floating-point arithmetic to increment the minor version ((0.9 * 10 + 1) / 10 = 1.0), which produced 1.0 instead of the expected 0.10. Replace with awk-based string splitting that treats major and minor as separate integers, preserving the major.minor format across all version numbers. Ref: EC-1705 Signed-off-by: Rob Nester <rnester@redhat.com> Assisted-by: Cursor Made-with: Cursor
Contributor
Review Summary by QodoFix version bump and add comprehensive architecture backlog
WalkthroughsDescription• 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 Diagramflowchart 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"]
File Changes1. Makefile
|
📝 WalkthroughWalkthroughMakefile minor-version bump target updated to compute the next minor version using an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Code Review by Qodo
1. VERSION format not validated
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
Makefiledocs/ARCHITECTURE_BACKLOG.md
Comment on lines
+454
to
+485
| | Value | Location | Description | | ||
| |-------|----------|-------------| | ||
| | 90 days | `evaluator/conftest_evaluator.go` | `effectiveOnTimeout` | | ||
| | 7 days | `cmd/validate/vsa.go` | `DefaultVSAExpiration` | | ||
| | 30 min | `cmd/validate/vsa.go` | `DefaultTimeoutDuration` | | ||
| | 30s | Multiple VSA files | Rekor timeout | | ||
|
|
||
| ### 5.2 URLs and Defaults | ||
| | Value | Location | Description | | ||
| |-------|----------|-------------| | ||
| | `https://rekor.sigstore.dev` | Multiple VSA files | Repeated 3+ times | | ||
| | `./vsa-upload` | `storage.go` | Local upload default | | ||
| | `"."` | `file_retriever.go` | File retriever default | | ||
| | GitHub/GitLab hints | `conftest_evaluator.go` ~463 | URL detection | | ||
|
|
||
| ### 5.3 Worker/Concurrency Defaults | ||
| | Value | Location | Description | | ||
| |-------|----------|-------------| | ||
| | 5 | `cmd/validate/vsa.go`, `image.go` | Default workers | | ||
| | 8 | `rekor_retriever.go` | Rekor workers | | ||
| | 64 | `rekor_retriever.go` | Rekor worker cap | | ||
|
|
||
| ### 5.4 File Permissions | ||
| | Value | Location | Description | | ||
| |-------|----------|-------------| | ||
| | 0755 | Multiple | Directory mode | | ||
| | 0600 | `vsa/writer.go` | Predicate JSON | | ||
| | 0644 | `vsa/attest.go` | Envelope | | ||
| | 0444 | `conftest_evaluator.go` | Data files | | ||
|
|
||
| ### 5.5 Scoring Weights | ||
| | Value | Location | Description | |
There was a problem hiding this comment.
Fix markdownlint MD058 table spacing violations.
The file has multiple “tables should be surrounded by blank lines” warnings. A single pass adding blank lines before/after each affected table will clear these lint findings.
Also applies to: 671-671, 679-679, 686-686, 697-697, 711-711, 748-748, 756-756, 763-763
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 454-454: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 462-462: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 470-470: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 477-477: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 485-485: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ARCHITECTURE_BACKLOG.md` around lines 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.
Comment on lines
+495
to
+519
| ### Source Files Over 500 Lines | ||
|
|
||
| | File | Lines | Primary Issue | | ||
| |------|-------|---------------| | ||
| | `cmd/validate/vsa.go` | 1906 | 16 structs; CLI + logic mixed | | ||
| | `internal/rego/oci/oci.go` | 1642 | All OCI rego functions | | ||
| | `internal/evaluator/conftest_evaluator.go` | 1271 | 5+ responsibilities | | ||
| | `internal/evaluator/filters.go` | 1206 | 3 filter mechanisms | | ||
| | `internal/validate/vsa/vsa.go` | 983 | Core + helpers mixed | | ||
| | `internal/policy/equivalence/equivalence.go` | 904 | Normalization + diffing | | ||
| | `internal/validate/vsa/rekor_retriever.go` | 755 | Retrieval + parsing | | ||
| | `cmd/validate/image.go` | 762 | CLI + validation mixed | | ||
| | `internal/policy/policy.go` | 686 | Policy + options + time | | ||
| | `internal/applicationsnapshot/report.go` | 421 | Multiple report formats | | ||
| | `internal/rego/sigstore/sigstore.go` | 396 | Verification functions | | ||
| | `internal/validate/vsa/storage_rekor.go` | 354 | Upload logic | | ||
| | `internal/output/output.go` | 349 | Multiple output formats | | ||
| | `internal/applicationsnapshot/input.go` | 333 | Input parsing | | ||
| | `internal/tracker/tracker.go` | 332 | Bundle tracking | | ||
| | `internal/opa/rule/rule.go` | 313 | Rule extraction | | ||
| | `internal/policy/source/source.go` | 295 | Source handling | | ||
| | `internal/validate/vsa/result.go` | 294 | Result types | | ||
| | `cmd/validate/input.go` | 286 | CLI + validation | | ||
| | `internal/validate/vsa/errors.go` | 269 | Error types | | ||
|
|
There was a problem hiding this comment.
The “Source Files Over 500 Lines” table includes multiple files below 500 LOC.
This section’s heading and data conflict (e.g., Line 508 421, Line 509 396, Line 510 354, etc.), which makes the backlog metrics and prioritization unreliable.
Please either (a) rename the section to match the actual threshold used, or (b) remove sub-500 entries from this table and keep them in a separate bucket.
🤖 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" 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.
Contributor
Author
|
Jira: EC-1726 |
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
force-pushed
the
ai_improvement_suggestions
branch
from
f6d5fd1 to
4003c0c
Compare
There was a problem hiding this comment.
🧹 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
>=500entries 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
📒 Files selected for processing (1)
docs/ARCHITECTURE_BACKLOG.md
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
1 participant
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.
Summary
docs/ARCHITECTURE_BACKLOG.mddocumenting identified architectural issues, technical debt, and improvement opportunitiesKey Findings
Highlights
EffectiveTime()value receiver bug, worker count off-by-one, result duplication in filtersUploadVSAEnvelopealways returns nil,prepareDataDirsignores walk errorsconftest_evaluator.gomixes 10+ responsibilities, two parallel pre-filtering mechanismsTest Plan
Ref: EC-9999