Redesign logging and error handling

[fgiudici]

Summary

• FilterByPackageNames now returns a PackagesNotFoundError (typed error) instead of []string, enabling callers to use errors.As for structured handling.
• Missing packages abort with exit code 3; with --permissive, missing packages log a warning per name and processing continues with the found subset.
• reworked logging:
  • slog (structured JSON logs) now always goes to stdout (operational info like "fetched N products")
  • -l flag now redirects validation/filtering reports (instead of slog) to a file (defaults to stderr when -l is not specified)

Exit codes

Code	Meaning
0	Success
1	Fatal error
2	No FBC data generated
3	Requested packages not found in PLCC data (without --permissive)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:02 PM UTC · Completed 4:15 PM UTC
Commit: 4d5ca93 · View workflow run →

[fullsend-ai-review]

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies a governance/infrastructure file (AGENTS.md) without a linked issue providing authorization. Human approval is always required for protected-path changes, and the absence of a linked issue means the justification for modifying this file cannot be verified against an authorized work item.
  Remediation: Link an issue that authorizes the AGENTS.md changes, or obtain explicit human approval for the protected-path modification.

Medium

• [missing-authorization] — Non-trivial enhancement (8 files changed across production code, tests, and documentation) with no linked issue. The PR redesigns logging architecture (slog to stdout, -l flag semantics), error handling patterns (typed errors, new exit code 3), and validation mode semantics (--permissive behavior). These are significant architectural decisions that benefit from tracked authorization.

• [breaking-api-change] pkg/plcc/plcc.go:148 — The exported method FilterByPackageNames signature changed from returning []string to returning error (PackagesNotFoundError). While this package appears to be used only internally by cmd/plcc2fbc, the method is exported and technically importable by external consumers. Consider moving the package to internal/plcc/ if external use is not intended.

Low

• [error-handling] cmd/plcc2fbc/main.go:41 — In main(), the PackagesNotFoundError case prints a hardcoded generic message ("Error: requested packages not found in PLCC data") to stderr instead of using err.Error() which includes the specific missing package names via strings.Join. The user must check structured JSON logs on stdout to learn which packages were missing.

• [cli-exit-code-change] cmd/plcc2fbc/main.go:43 — New exit code 3 for missing -p packages (without --permissive). Scripts checking for specific exit codes may need updates. Well-documented in the updated README and AGENTS.md.

• [new-exported-type] pkg/plcc/plcc.go:30 — New exported error type PackagesNotFoundError with exported Names []string field extends the public API surface. See also: [breaking-api-change] finding on FilterByPackageNames.

• [edge-case] cmd/plcc2fbc/main.go — writeSplit now returns len(products) as the count instead of summing GenerateFBC return values. Equivalent under the new count != 1 assertion, but a semantic shift from the previous behavior.

Previous run

Review

Reason: stale-head

The review agent reviewed commit 92d83ceb190a9744ec55cd63fb1dc01a45684f3d but the PR HEAD is now 0cd2de4a982839b8c40e58ef7b45eb56b74f3c1b. This review was discarded to avoid approving unreviewed code.

Previous run (2)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies a governance file (AGENTS.md) but has no linked issue to justify the change. The AGENTS.md updates reflect the new error handling and logging model, but protected-path changes require human approval regardless of context. Link an issue documenting the rationale for these infrastructure file changes.

Medium

• [breaking-api-change] pkg/plcc/plcc.go:139 — Breaking change to exported method signature: FilterByPackageNames(names []string) []string changed to FilterByPackageNames(names []string) error. While no external consumers were found (this is a CLI tool), the exported Go API contract changed without a major version bump. If external consumers exist, this will break compilation.
  Remediation: Document in the PR that this module has no external importers. If external consumers exist, provide a deprecation path or bump the module to v2.

• [new-exported-type] pkg/plcc/plcc.go:28 — New exported error type PackagesNotFoundError with public field Names []string establishes a new API contract. Once published, callers may depend on type assertions and field access. Consider whether Names should be unexported with an accessor method to preserve future flexibility.

• [cli-flag-semantics-change] cmd/plcc2fbc/main.go — The -l/--log flag semantics changed from "write slog operational logs to file (default: stdout)" to "write validation/filtering report to file (default: stderr)". This is a breaking change for users or scripts relying on the old behavior. The Makefile correctly reflects the new invocation pattern (swapping -l and stdout/stderr redirects), but downstream consumers outside this repo may be affected.
  Remediation: Document the breaking change clearly. Consider whether a new flag name (e.g., --report) would avoid confusion.

• [cli-exit-code-change] cmd/plcc2fbc/main.go — New exit code 3 added for package-not-found errors. Previously, all non-output errors exited with code 1 (via log.Fatal). Scripts checking $? -eq 1 for errors will not catch exit code 3. The distinction between exit 2 (no FBC output after validation) and exit 3 (requested packages not in source data) is reasonable but represents a compatibility change.
  Remediation: The exit codes are documented in README.md and AGENTS.md, which is good. Ensure any CI pipelines or wrapper scripts are updated.

Low

• [logic-error] cmd/plcc2fbc/main.go:173 — In writeSplit, the check changed from count == 0 to count != 1. The new check is stricter (rejects count > 1 as well), which is an improvement. However, the error message "produced N FBC blobs, expected 1" is less descriptive than the old "produced no FBC data" for the count==0 case. Consider separate handling for clarity.

• [missing-authorization] This PR makes non-trivial structural changes but has no linked issue. The PR body is descriptive, but linking an issue would provide authorization context.

• [function-signature] cmd/plcc2fbc/main.go:152 — writeSplit returns len(products) while writeFile returns blobCount. These are semantically equivalent given the count != 1 invariant in split mode, but documenting the return value semantics would improve clarity.

Previous run (3)

Review

Findings

High

• [breaking-api] pkg/plcc/plcc.go:148 — FilterByPackageNames changes its return type from []string to error. This is a breaking change for any external code importing the pkg/plcc package. Callers that assign the return value (e.g., notFound := catalog.FilterByPackageNames(names)) will experience compile-time breakage.
  Remediation: Consider preserving backward compatibility by creating a new method (e.g., FilterByPackageNamesE), bumping the module major version, or documenting this as a breaking change in release notes. If the package is internal-only, consider moving it to internal/.

• [protected-path] AGENTS.md — This PR modifies a protected governance file (AGENTS.md) without a linked issue. Protected-path changes always require human approval regardless of content. The AGENTS.md updates are documentation reflecting code changes, but the absence of a linked issue means there is no traceable authorization for modifying governance files.
  Remediation: Link an issue that authorizes the changes in this PR, including the AGENTS.md updates.

Medium

• [missing-authorization] This PR implements non-trivial changes (192 additions, 85 deletions across 8 files) including new error types, exit code behavior, and I/O stream redesign without a linked issue. Non-trivial changes require explicit authorization.

• [api-contract] pkg/plcc/plcc.go:148 — FilterByPackageNames mutates the catalog (filters c.Data to matched products) before returning a PackagesNotFoundError. The function both succeeds (filtering is applied) and returns an error (some names were not found). The doc comment does not clarify this partial-success behavior. The caller in loadAndValidate relies on this (continues with the filtered catalog in permissive mode). See also: [breaking-api] finding at this location.

• [breaking-cli] cmd/plcc2fbc/main.go — Two CLI behavioral changes that may break downstream automation: (1) New exit code 3 for missing -p packages — previously these were logged as warnings and execution continued. (2) The -l/--log flag now redirects validation/filtering reports (default: stderr) instead of operational slog logs (default was: stdout). Users passing -l will get different content in their log files without warning.

• [error-message-formatting] cmd/plcc2fbc/main.go:38 — The PackagesNotFoundError case in main() prints a hardcoded message "Error: requested packages not found in PLCC data" without including the actual error, which contains the specific missing package names. The per-package details are logged via slog.Error on stdout, but the stderr message loses useful information.

Low

• [edge-case] cmd/plcc2fbc/main.go — In loadAndValidate, the !errors.As(err, &pkgErr) branch after FilterByPackageNames is currently unreachable since FilterByPackageNames can only return nil or *PackagesNotFoundError. This is defensive programming that guards against future changes.

• [test-adequacy] cmd/plcc2fbc/main_test.go — The "permissive partial match warns but succeeds" test verifies output content but does not verify that warnings were emitted for missing packages. The "partial match errors by default" test confirms PackagesNotFoundError but does not assert on pkgErr.Names.

• [scope-creep] cmd/plcc2fbc/main.go — The PR title suggests logging redesign, but changes extend to exit codes, error type hierarchy, and --permissive flag semantics. These are arguably within "error handling" scope as stated in the title, but the breadth of behavioral changes warrants clear documentation in the PR description.

Previous run (4)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a protected governance file. No linked issue provides authorization for these changes. Human approval is always required for protected-path changes, regardless of context.

Medium

• [error-wrapping-pattern] cmd/plcc2fbc/main.go:132 — The fmt.Errorf wrapping of NewPackageWriter's error duplicates the format value and allowed-values list already present in the underlying error, producing a cluttered double message like: invalid output format "xml" (allowed: json, json-pretty, yaml): unsupported format "xml": must be "json", "json-pretty", or "yaml".
  Remediation: Simplify to return fmt.Errorf("invalid output format: %w", err) to avoid repeating info from the wrapped error.

• [behavioral-coherence] README.md:26 — The --permissive flag is extended to cover missing -p packages, but its documented purpose is "Keep packages that fail PLCC validation instead of filtering them out." Missing packages are a user-input error, not a validation failure. This semantic overloading dilutes the flag's clear purpose.
  Remediation: Consider a separate flag (e.g., --ignore-missing-packages) or document why missing packages should be treated as a validation concern.

• [breaking-change] pkg/plcc/plcc.go:139 — The exported method FilterByPackageNames has a backward-incompatible signature change from returning []string to error. This is a pre-v1 module with no tagged releases and only one internal consumer, so the practical risk is low, but the break should be noted in the PR description for tracking.

Low

• [error-handling] cmd/plcc2fbc/main.go:120 — Users relying only on stderr will not see which specific packages were missing (stderr gets a generic message; per-package details go to stdout via slog). Documented as intentional but worth confirming as desired UX.

• [test-adequacy] cmd/plcc2fbc/main_test.go — No test covers the --permissive partial-match case in --split mode, unlike the analogous non-split "permissive partial match warns but succeeds" test.

• [logging-consistency] cmd/plcc2fbc/main.go:91 — Variable names logPath and lf still suggest "log file" but the -l flag now redirects the validation/filtering report. Consider renaming to reportPath/reportFile for clarity.

• [exit-code-coherence] README.md — Exit codes 2 and 3 are semantically distinct (data-quality issue vs user-input error), but the distinction could be documented more explicitly for CI/CD consumers.

• [naming-coherence] pkg/plcc/plcc.go — PackagesNotFoundError naming is slightly ambiguous — could refer to FBC packages or PLCC products. Consider RequestedPackagesNotFoundError for clarity.

Previous run (5)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies a protected governance file (AGENTS.md) without a linked issue. Protected paths require explicit authorization via a linked issue and human approval, regardless of the nature of the changes. The AGENTS.md changes update documentation to reflect code changes (exit codes, logging model, new types), which is appropriate, but human review of governance file modifications is always required.
  Remediation: Link an issue authorizing the changes or obtain explicit human approval for the AGENTS.md modifications.

Medium

• [breaking-api] pkg/plcc/plcc.go:139 — FilterByPackageNames signature changed from returning []string to error. While this module is at v0 (no version tags, no Go compatibility guarantee) and no external imports were found, this is a source-incompatible change to an exported function in a public package (pkg/plcc).

• [architectural-coherence] pkg/plcc/plcc.go — Returning error from FilterByPackageNames changes its semantics from a filter operation (returns unmatched names for caller to decide) to a validation-like operation (returns error on any mismatch). The caller must now unwrap PackagesNotFoundError to retrieve the missing names, adding complexity. Consider whether the []string return better preserves the filter/validation boundary documented in AGENTS.md.

• [architectural-coherence] cmd/plcc2fbc/main.go — The -l/--log flag semantics change from redirecting slog output (default: stdout) to redirecting report/validation output (default: stderr). This is a user-facing behavioral change — existing workflows relying on -l to capture structured JSON logs will silently lose that output to stdout instead. The Makefile update confirms the impact: the shell redirections are swapped.

• [slog-usage-pattern] cmd/plcc2fbc/main.go:88 — slog.SetDefault now always targets stdout regardless of -l. Previously, all structured logging respected the -l flag. The new architecture (slog→stdout, reports→stderr/-l file) is documented in the AGENTS.md update but represents a breaking behavioral change.
  See also: [architectural-coherence] finding on -l flag semantics.

• [error-handling-pattern] cmd/plcc2fbc/main.go:40 — Replaced log.Fatal(err) with fmt.Fprintln(os.Stderr, ...) + os.Exit(N). Necessary to support differentiated exit codes (1/2/3), but introduces a new error reporting pattern. The old log.Fatal logged with timestamp and prefix; the new pattern prints plain "Error: ..." to stderr.

• [error-logging-pattern] cmd/plcc2fbc/main.go:119 — In strict mode, missing package names are logged via slog.Error in run() (line ~119) before the error propagates to main(). In permissive mode, they are logged via slog.Warn in loadAndValidate (line ~244). Both go to stdout; neither appears in the -l report file. The different log levels for the same event in different modes is intentional but worth documenting.

• [parameter-naming] cmd/plcc2fbc/main.go:88 — Variable renamed from logWriter to reportWriter while the CLI flag remains --log/-l, creating a disconnect between user-facing and internal naming.

• [breaking-cli] cmd/plcc2fbc/main.go — New exit code 3 for "packages not found." Documented in README.md and AGENTS.md updates within this PR. Existing scripts checking $? -ne 0 are unaffected, but scripts matching specific codes (e.g., $? -eq 2 for "no output") may need awareness of the new code.

Low

• [missing-authorization] No linked issue for this non-trivial enhancement (192 additions, 85 deletions, 8 files).
• [scope-creep] PR scope extends beyond title ("typed error + centralize error logging") to include exit codes, logging architecture redesign, and function signature changes. The changes are cohesive but the title understates scope.
• [test-inadequate] cmd/plcc2fbc/main_test.go:168 — "with log file" test asserts non-empty output but relies on test data (plcc.json) producing validation results. A targeted test with crafted input would be more robust.

Previous run (6)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, a governance/infrastructure file that requires human approval. The PR has no linked issue and does not explain why the protected file is being changed (beyond reflecting code changes). Human review is required for all protected-path modifications regardless of context.
  Remediation: Link an issue that authorizes the changes to AGENTS.md, or add justification to the PR description explaining why the governance file needs updating.

Medium

• [breaking-change-exported-method] pkg/plcc/plcc.go:139 — The exported method FilterByPackageNames on exported type Catalog changes return type from []string to error. Any external caller using the []string return will fail to compile. The module is at v0 (no major version suffix in go.mod), so breaking changes are permitted per Go semver, but should still be documented.
  Remediation: Document the breaking change in the PR description or a CHANGELOG. Check pkg.go.dev and GitHub dependency graph for external consumers before merging.

• [scope-creep] cmd/plcc2fbc/main.go — The PR mixes two concerns: (1) typed error handling for missing packages (PackagesNotFoundError, exit code 3), and (2) general error-handling cleanup in the write path (writeSplit/writeFile signature changes, slog removal, success logging relocation). These are related but separable changes that increase review surface area.

• [exit-code-coherence] cmd/plcc2fbc/main.go — Exit code 3 creates a third category alongside 1 and 2. Both exit codes 2 and 3 represent "no output produced" scenarios. The distinction (2 = all filtered out by validation, 3 = requested -p packages not found) is meaningful for debugging but could confuse automation that consumes exit codes.

• [error-logging-centralization] cmd/plcc2fbc/main.go:114 — The PR removes slog.Error calls before returning errors in several places, returning errors directly instead. However, some slog.Error calls remain (e.g., catalog/product validation logging). The error logging pattern migration is partial.

• [error-handling-convention] cmd/plcc2fbc/main.go:39 — The error handling in main() changes from log.Fatal to fmt.Fprintln(os.Stderr, ...) + os.Exit(1). While functionally equivalent, this introduces fmt.Fprintln as a new stderr output mechanism alongside slog, creating two different output patterns.

Low

• [error handling] cmd/plcc2fbc/main.go:111 — When loadAndValidate returns a PackagesNotFoundError in strict mode, run() logs missing package names via slog.Error, then main() calls os.Exit(3) without printing anything to stderr. If slog output goes to a log file, the user sees only exit code 3 with no indication of which packages were missing.

• [function-signature-consistency] cmd/plcc2fbc/main.go:153 — writeSplit uses unnamed return (int, error) while writeFile uses named return (count int, err error). The difference is functionally motivated (writeFile needs named returns for a defer-close pattern), but the inconsistency between sibling functions is worth noting.

• [architectural-alignment] pkg/plcc/plcc.go — PackagesNotFoundError is the first custom typed error in the codebase, establishing a new pattern alongside existing sentinel errors (errNoFBCOutput) and plain errors (fmt.Errorf). Consider documenting when each error pattern should be used.

Previous run (7)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a protected governance file. The PR has no linked issue providing authorization for this change. Protected-path changes always require human approval.
  Remediation: Link an issue that authorizes the changes to AGENTS.md, or have a maintainer explicitly approve the protected-path modification.

Medium

• [breaking-api] pkg/plcc/plcc.go:148 — FilterByPackageNames signature changed from func (c *Catalog) FilterByPackageNames(names []string) []string to func (c *Catalog) FilterByPackageNames(names []string) error. This is a source-incompatible change for any external caller assigning the result to []string. The pkg/plcc/doc.go and pkg/fbc/doc.go document this as a library with public API surface ("Library callers should use [Translate]"). The module is pre-v1.0 (no version tags), so Go module compatibility guarantees do not formally apply, but downstream consumers will still need to update their code.
  Remediation: Document the signature change in release notes. If external consumers exist, consider a deprecation period or a new method name.

• [breaking-cli] cmd/plcc2fbc/main.go:42 — New exit code 3 added for missing -p packages. Previously these cases resulted in exit code 2 (no FBC data generated). External scripts or CI pipelines that check for specific exit codes may need updating. The new exit code is documented in the PR (README.md, AGENTS.md), but existing consumers of the CLI may not expect it.
  Remediation: Document the new exit code in release notes and any migration guides.

Previous run (8)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies a protected governance file (AGENTS.md) without a linked issue providing justification. Protected-path changes always require human approval. The updates appear to be routine documentation of the code changes (new error type, exit code 3, logging pattern), but human review is required to confirm accuracy.
  Remediation: Link this PR to an issue that authorizes the changes, or obtain explicit human approval for the AGENTS.md modifications.

Medium

• [logic-error] cmd/plcc2fbc/main.go:142 — After refactoring, run() logs slog.Info("wrote FBC data", "count", len(catalog.Data), ...) which reports the number of input products, not the number of FBC blobs actually written. The old code in writeFile logged blobCount from GenerateFBC(), which reflects packages surviving Translate filtering. Since Translate can reject products via DefaultFilters, len(catalog.Data) may overcount the actual output.
  Remediation: Have writeFile/writeSplit return the actual blob count (e.g., as an additional int return value), and use that in the slog.Info call in run(). Alternatively, keep the slog.Info inside those functions where the accurate count is available.

• [error-handling-gap] cmd/plcc2fbc/main.go:40 — The PR centralizes error logging in main() with a catch-all slog.Error("fatal error", ...) before os.Exit(1), and removes slog.Error from writeFile, writeSplit, and loadAndValidate. However, two error paths in run() retain their own slog.Error calls: the dumpPLCC failure (line 114) and NewPackageWriter failure (line 123). These paths will produce double error logging. This also contradicts the AGENTS.md gotcha added by this PR: "inner functions return errors without logging; main() handles all slog.Error/slog.Warn output."
  Remediation: Remove the remaining slog.Error calls from run() for the dumpPLCC and NewPackageWriter error paths to match the stated pattern, or clarify in AGENTS.md that run() is excluded from the "inner functions" rule.

Low

• [error-handling-gap] cmd/plcc2fbc/main.go:233 — In the permissive path of loadAndValidate, if FilterByPackageNames returns an error that is not *PackagesNotFoundError, it is silently swallowed. Currently FilterByPackageNames can only return nil or *PackagesNotFoundError, so this is safe today. However, if a future change adds another error return path, the non-strict branch would silently ignore it.

• [test-inadequate] cmd/plcc2fbc/main_test.go — The new "permissive partial match warns but succeeds" test verifies that output contains rhacs-operator but does not verify that a warning was logged for the missing nonexistent package. The warning-on-permissive behavior is the key semantic being introduced but is not actually asserted.

• [missing-authorization] — This PR introduces non-trivial changes (typed error, new exit code, logging centralization) without a linked issue. Consider linking to an issue documenting the design decisions.

• [breaking-api-change] pkg/plcc/plcc.go:148 — FilterByPackageNames is an exported method whose return type changed from []string to error. This is a backward-incompatible change. The module appears to be pre-v1 (no version suffix in module path), so breaking changes are expected per Go module conventions, but worth noting for any potential external consumers.

Previous run (9)

Review

Findings

High

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, which is a protected governance file. The PR has no linked issue providing authorization for changes to protected infrastructure files. Human approval is required regardless of context, and the lack of a linked issue means reviewers cannot verify that the change was authorized.
  Remediation: Link this PR to an issue that authorizes changes to AGENTS.md, or obtain explicit human approval for the protected-path modifications.

Medium

• [logic-error] cmd/plcc2fbc/main.go:136 — The slog.Info("wrote split FBC data", ...) log line was moved from writeSplit to run(), changing the logged count from totalCount (sum of FBC blob counts per product returned by GenerateFBC) to len(catalog.Data) (number of input products). These values can differ because GenerateFBC calls Translate with DefaultFilters(), which can reject products. The log message now reports a potentially inflated count.
  Remediation: Either keep the count aggregation inside writeSplit and return it as a second return value, or rename the log field to products to clarify it is an input count, not an output blob count.

• [logic-error] cmd/plcc2fbc/main.go:141 — Same issue for the non-split path: the slog.Info("wrote FBC data", ...) was moved from writeFile (where it logged blobCount from GenerateFBC) to run() (where it logs len(catalog.Data)). The blobCount reflects how many packages survived the FBC filter pipeline, while len(catalog.Data) is the pre-filter input count.
  Remediation: Have writeFile return the blob count, or keep the log line inside writeFile where blobCount is available.

• [backward-incompatible-api-change] pkg/plcc/plcc.go:148 — The exported method Catalog.FilterByPackageNames changes its return type from []string to error. This is a backward-incompatible change to the public API. While this is a v0.x module (no backward-compatibility guarantee), the change also creates an inconsistency with FilterPackages() which filters in-place and returns nothing. See also: [architectural-inconsistency] finding below.
  Remediation: Consider returning ([]string, error) to preserve the original information while adding error semantics, or document why FilterByPackageNames and FilterPackages have different return patterns.

• [logging-convention] cmd/plcc2fbc/main.go:114 — The PR centralizes error logging in main(), removing slog.Error calls from writeFile, writeSplit, and loadAndValidate. However, the dumpPLCC error path at line 114 still logs slog.Error("failed to write PLCC dump", ...) before returning, creating an inconsistency with the new pattern.
  Remediation: Remove the slog.Error at line 114 to fully centralize error logging, or document why this path is intentionally different.

Low

• [error-handling] cmd/plcc2fbc/main.go:229 — In loadAndValidate, when strict is false (permissive mode) and FilterByPackageNames returns an error that is NOT a *PackagesNotFoundError, the error is silently swallowed. Currently FilterByPackageNames can only return *PackagesNotFoundError or nil, so this is unreachable today, but the defensive pattern would be to log or return unknown errors.

• [error-handling-pattern] cmd/plcc2fbc/main.go:232 — loadAndValidate uses a direct type assertion (pkgErr, ok := err.(*plcc.PackagesNotFoundError)) while main() uses errors.As. Using errors.As consistently is more idiomatic Go and handles wrapped errors correctly.

• [logging-convention] cmd/plcc2fbc/main.go:133 — Success logging was moved to run() after writeFile/writeSplit return, while error logging was moved out of these functions. This creates a layered approach (success in run(), errors in main()) which is a reasonable design choice but should be documented as the intended pattern.

• [architectural-inconsistency] cmd/plcc2fbc/main.go — FilterByPackageNames now returns an error for missing packages, blurring the distinction between filtering (silently removes data) and validation (produces errors). FilterPackages() returns nothing. See also: [backward-incompatible-api-change] finding above.

Previous run (10)

Review

Reason: stale-head

The review agent reviewed commit b8c975da01042d33d9f5c27afdfc7eb9a3339e06 but the PR HEAD is now a3cae0943beac24558c196933b5527cf3696be55. This review was discarded to avoid approving unreviewed code.

Previous run (11)

Review

Findings

High

• [protected-path] AGENTS.md — Protected governance file modified without a linked issue. The PR updates the Gotchas section to document exit code 3, which is a reasonable documentation update, but AGENTS.md requires human approval and the PR lacks a linked issue to establish authorization for the change.
  Remediation: Link an issue that authorizes the AGENTS.md update, or have a maintainer explicitly approve the protected-path change.

Medium

• [nil-deref] cmd/plcc2fbc/main.go:233 — Bare type assertion err.(*plcc.PackagesNotFoundError) on the non-strict path will panic if FilterByPackageNames ever returns a different error type. Currently safe because FilterByPackageNames only returns nil or *PackagesNotFoundError, but fragile and inconsistent with the errors.As usage in main() and tests.
  Remediation: Use errors.As or the comma-ok form: if pkgErr, ok := err.(*plcc.PackagesNotFoundError); ok { ... }

• [missing-behavior-documentation] README.md — Exit codes are not documented in the user-facing README. This PR introduces exit code 3 for packages-not-found with --strict; exit codes 1 and 2 were also previously undocumented.
  Remediation: Add an Exit Codes section to README.md documenting codes 0, 1, 2, and 3.

• [incomplete-flag-documentation] README.md — The --strict flag description does not mention the new exit code 3 behavior when packages specified via --package are not found in PLCC data.
  Remediation: Update the --strict flag description to mention exit code 3 for missing packages.

Low

• [logging-consistency] cmd/plcc2fbc/main.go:48 — The fallback error path uses slog.Error(err.Error()) instead of the structured slog.Error("description", "error", err) pattern used elsewhere in the codebase.

• [error-message-clarity] cmd/plcc2fbc/main.go:137 — Success logging for the split path dropped the count attribute that was present in the original, reducing operational visibility into how many packages were written.

• [missing-behavior-documentation] docs/VALIDATION_RULES.md — The summary of filtering behavior table does not include a row for the packages-not-found scenario.

• [naming-consistency] cmd/plcc2fbc/main.go:37 — The distinction between errNoFBCOutput (zero-output condition) and PackagesNotFoundError (missing input packages) could benefit from a brief comment above errNoFBCOutput.

---

Labels: PR enhances error handling with typed errors and new exit codes.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:06 PM UTC · Completed 5:18 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:21 PM UTC · Completed 5:33 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:39 PM UTC · Completed 5:53 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:31 AM UTC · Completed 8:42 AM UTC
Commit: feb9bda · View workflow run →

[asergienk]

latest changes LGTM

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:02 PM UTC · Completed 2:21 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:39 PM UTC · Completed 3:46 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:42 PM UTC · Completed 5:49 PM UTC
Commit: feb9bda · View workflow run →

[perdasilva]

Code review: 3 findings (1 bug, 1 convention, 1 cleanup). Details in inline comments.

[perdasilva]

Bug: double error output for generic errors (exit code 1)

For any error that isn't PackagesNotFoundError or errNoFBCOutput, run() logs it via slog.Error("fatal error", ...) (lines 120, 128, 138, 152) and then returns it here, where main() prints it again via fmt.Fprintln. Without -l, the user sees the same error twice in different formats on different streams (JSON on stdout, plain text on stderr).

Suggestion: either skip the slog.Error in run() for errors that will reach this default case, or remove fmt.Fprintln here and rely on slog alone. Alternatively, limit the fmt.Fprintln fallback to errors that occur before slog.SetDefault (line 101).

[fgiudici]

Reworked logging:

• slog (structured JSON logs) now always goes to stdout: operational info like "fetched N products", "filtered packages", "wrote FBC data"
• -l flag now redirects validation/filtering reports (report.LogResults JSON lines) to a file: defaults to stderr when -l is not specified
  This cleanly separates the two output channels: slog for operators watching the tool run, reports for downstream tooling or review.

For the error logging, we now always print them on stderr by main() via the fmt.Fprintln, keeping the error message brief.
slog.Errors are printed too in some cases, but in json format together with the other json logs on stdout. There all details are included.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:08 AM UTC · Completed 11:25 AM UTC
Commit: feb9bda · View workflow run →

[fgiudici]

[breaking-api] FilterByPackageNames signature change

Pre-v1 module, no external consumers. Intentional.

[architectural-coherence] FilterByPackageNames returning error vs []string

By design: the caller shouldn't silently ignore missing packages. The error forces explicit handling (strict: fail, permissive: warn). The old []string return was easy to discard.

[architectural-coherence] -l flag semantics change

Intentional. The previous model was backwards: operational logs (slog) belong on stdout, not in a file. The -l flag now captures the validation report, which is the output users actually want to save separately.

[slog-usage-pattern] slog always to stdout

By design: see above. Structured JSON logs on stdout is the standard pattern for CLI tools. Validation reports are a separate output channel.

[error-handling-pattern] log.Fatal → fmt.Fprintln

Necessary to support differentiated exit codes (1/2/3). log.Fatal always exits 1 and used the log package which we don't use (all structured logging is log/slog).

[error-logging-pattern] Different log levels for strict vs permissive

Intentional. In default strict mode, missing packages are an error (the run fails). In permissive mode, they're a warning (the run continues with the found subset). Different severity = different log level. The permissive mode is more a debug feature to us the tool to still produce some data.

[parameter-naming] reportWriter vs --log flag

The flag name was intentionally kept as --log for simplicity (-l). The internal variable name reflects what it actually carries. The flag description clarifies the semantics.

[breaking-cli] New exit code 3

Documented in README exit codes table. The distinction between "all filtered out" (2) and "requested packages not found" (3) enables better automation.

[asergienk]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 7:38 PM UTC · Completed 7:57 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:20 AM UTC · Completed 10:40 AM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:28 AM UTC · Completed 11:45 AM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:05 PM UTC · Completed 12:20 PM UTC
Commit: feb9bda · View workflow run →

[fullsend-ai-review]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:23 PM UTC · Completed 12:37 PM UTC
Commit: 02c45b9 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fgiudici]

I think this PR ready. it took more than planned and too many back and forth rounds for the actual changes in it.
There is some duplicate error logging by the current design: slog stream on stdout logs everything in json, and logs many errors too (on stdout).
Main() will drop a short message on STDERR with all the details stripped down (when the error is in the json logging too).
I don't think it is worth to iterate again on this: I like current logging state, if we want to change it makes sense to have a separate PR for that.
I would force merge this at this point, but I cannot 😢
If someone could at least drop a blind approval I would take full responsibility for merging this.

Addressable insights have been tackled, inline comments have inline replies.
What is remaining are complaints about API changes, unauthorized changes (missing tracking issue?) and minor notes.