Fix review comments: structured logging, type safety, schema, and typos#26
Merged
Fix review comments: structured logging, type safety, schema, and typos#26
Conversation
Merged
Co-authored-by: alvagante <283804+alvagante@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
[WIP] Fix calculation in Users and RBAC functionality
Fix review comments: structured logging, type safety, schema, and typos
Mar 3, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses feedback from PR #20 by standardizing backend logging, tightening type usage in integration services, correcting a DB migration constraint, and fixing small documentation/config typos across the repo.
Changes:
- Replace ad-hoc RBAC/security logging with
LoggerService-based structured logs and adjust sanitization behavior for deep nesting. - Fix DB migration/schema alignment (
executions.typeCHECK constraint) and remove a non-effective SQLite PRAGMA. - Reduce type duplication / formatting issues in integration services and correct minor typos in docs/config.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| backend/src/middleware/securityMiddleware.ts | Adds LoggerService and logs when max sanitization depth is exceeded. |
| backend/src/middleware/rbacMiddleware.ts | Replaces console.warn/error with structured LoggerService logs. |
| backend/src/integrations/puppetdb/PuppetDBService.ts | Cleans up indentation and removes redundant client variable usage in getGroups(). |
| backend/src/integrations/bolt/BoltPlugin.ts | Removes leftover @todo JSDoc stub above getGroups(). |
| backend/src/integrations/ansible/AnsibleService.ts | Uses shared NodeGroup type instead of an inline anonymous return type. |
| backend/src/database/migrations/006_add_batch_executions.sql | Adds 'plan' to the executions.type CHECK constraint. |
| backend/src/database/MigrationRunner.ts | Changes migration progress output from console.warn to console.log. |
| backend/src/database/DatabaseService.ts | Removes PRAGMA page_size which is ignored for existing DBs. |
| backend/.env.example | Fixes “Allowd” → “Allowed”. |
| README.md | Fixes “RBC” → “RBAC”. |
| Dockerfile | Fixes “Awesaome” → “Awesome”. |
Comments suppressed due to low confidence (1)
backend/src/integrations/bolt/BoltPlugin.ts:380
- The JSDoc block and
getGroups()method are mis-indented compared to the rest of the class (extra leading spaces before*and beforeasync). Please reformat this section to match the surrounding 2-space indentation so the comment renders correctly and the file stays consistent with project formatting conventions.
/**
* Get groups from Bolt inventory
*
* Parses inventory.yaml to extract group definitions including:
* - Group names and targets
* - Nested groups (stored in metadata.hierarchy)
* - Variables and config (stored in metadata)
*
* @returns Array of node groups
*/
async getGroups(): Promise<NodeGroup[]> {
const complete = this.performanceMonitor.startTimer('bolt:getGroups');
Comment on lines
+213
to
+215
| console.log(`Applying migration: ${migration.filename}`); | ||
| await this.executeMigration(migration); | ||
| console.warn(`✓ Migration ${migration.filename} applied successfully`); | ||
| console.log(`✓ Migration ${migration.filename} applied successfully`); |
There was a problem hiding this comment.
console.log is disallowed by the repo’s ESLint no-console rule (only warn/error are allowed), so these lines will introduce lint warnings in CI. Please route this through LoggerService.info() (preferred for consistency) or add an eslint disable if direct console output is truly required.
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
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.
Addresses review feedback from PR #20 covering logging consistency, schema correctness, type duplication, indentation, and documentation typos.
Logging
rbacMiddleware.ts: Replaceconsole.warn/console.errorwithLoggerService(structuredcomponent/operation/metadatafields)securityMiddleware.ts: AddLoggerServiceimport; uselogger.warn()on max-depth exceeded and returnnullinstead of silently passing through unsanitized inputMigrationRunner.ts: Replace informationalconsole.warnwithconsole.logSchema / Database
006_add_batch_executions.sql: Add'plan'to thetypeCHECK constraint — was missing despiteExecutionRepository.tsaccepting itDatabaseService.ts: RemovePRAGMA page_size = 4096— silently ignored on existing databasesType Safety & Code Quality
AnsibleService.ts: ImportNodeGroupfrom../typesand useNodeGroup[]as return type forgetGroups()instead of an inline anonymous object typePuppetDBService.ts: Remove redundant intermediateconst client = this.client(already null-checked above); fix 6-space indentation ingetGroups()andcreateEnvironmentGroups()/createOSFamilyGroups()to match 2-space class conventionBoltPlugin.ts: Remove leftover@todoJSDoc stub abovegetGroups()implementationDocumentation / Config
.env.example:Allowd→AllowedDockerfile:Awesaome→AwesomeREADME.md:RBC→RBAC💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.