Remediation (by the book): Phase A/B fixes - #9
Merged
Conversation
The image ran `python service/diagnostic_service.py`, which puts /app/service (not /app) on sys.path, so the module's absolute `from service.logging_config import ...` failed with ModuleNotFoundError and the container crash-looped (blocking monitoring-service and api-gateway, which depend on it). Set PYTHONPATH=/app so the `service` package resolves. Verified live: the service comes up healthy and Prometheus scrapes it.
Both found by running the full stack live with the simulators: - MonitoringController.recordMetricsBatch built the MetricDataDTO with type/value/timestamp but never the band, and BatchMetricEntry had no band field — so every batched NR metric (DL/UL_THROUGHPUT, RSRP, SINR) from the edge bridge was stored as band=NONE, leaving the 5G dashboard's n28/n78 sections empty. Added the field and wired it through; verified live that N28/N78 now land. Regression test captures the DTO band. - edge-bridge Dockerfile used golang:1.21 while go.mod requires 1.23, so the image failed to build (`go >= 1.23.0`). Bumped to golang:1.23-alpine.
Full read-only audit of the whole repo, marking what should change for correctness, security, and maintainability — grounded in the reference library (Effective Java, 100 Go Mistakes, Seacord, Khorikov/GOOS, Brazil/ Observability Engineering, OAuth 2 in Action, Newman/Nygard, Kleppmann, Refactoring UI/Laws of UX, 3GPP/O-RAN/TMF). Phased by severity/risk. Honest provenance note: the 11-agent parallel audit hit the account session limit before returning line-by-line rows, so this is assembled from direct inspection + this session's deep working knowledge, with targeted detection scans; areas needing an exhaustive per-file pass are marked for re-run when budget resets. Headline themes: oversized Python/frontend modules (decomposition), and missing cross-service contract tests + CI never building the container images (which let three live bugs through this session).
Finished the planning via direct main-thread inspection (the parallel auditors were blocked by the account session limit). Every non-"(needs pass)" row is now verified by reading the code, with file:line + book ref: - C: frame.c/transport_tcp.c verified memory-safe; concrete tcp_send busy-loop + unchecked setsockopt/fcntl; transport_tls.c flagged for pass. - Go: bridge.go concurrency verified leak-safe; found the metrics-clobber logic bug; time.After idiom notes. - Auth: JWT alg-confusion prevented (good); found JwtValidator info-leak, missing clock-skew, missing iss/aud binding. - Notification: RestTemplate-without-timeout in Slack/PagerDuty (HIGH); 8 CompletableFuture sites to verify error-handling. - Monitoring: EJ-62 stringly-typed status/severity/band; Optional.get() misuse. - Python: 25 broad except in diagnostic_service; 128-line _register_routes. - Frontend: Reports.tsx missing loading/error states. Remaining "(needs pass)" blocks (per-file Java enumeration, TLS, Go adapters, refresh-token/lockout) are scoped for the auditors on reset.
Completed every remaining deep pass by direct inspection; removed all "(needs pass)" markers. New verified file:line findings added: - Auth (OAuth 2 in Action): refresh-token reuse-detection missing (replayed revoked token should revoke the family), verify→rotate TOCTOU double-spend race, tokens stored/looked-up in plaintext (hash them). - C TLS (Seacord): deprecated mbedTLS 2.x API (EOL); VERIFY_NONE opt-out foot-gun. Verified secure-by-default (TLS1.2+, VERIFY_REQUIRED, CA chain, get_verify_result checked, SNI hostname). - Go cloud client (Nygard): fixed retry delay (no backoff/jitter), retries non-idempotent POST, no ctx cancellation; unbounded upload buffer. - base-station (Kleppmann): 2x @ElementCollection(EAGER) N+1 risk; verified LAZY @manytoone and no entity equals/hashCode trap. - Infra: corrected earlier error — compose DOES set resource limits; the real issues are 6/8 Dockerfiles run as root, CI builds 0 images, 2 .dockerignore. - monitoring (EJ 62/55), notification (RestTemplate no-timeout), frontend (Reports.tsx states), tests (Awaitility not sleep; 20 verify()-based). Every row verified against the code; nothing deferred.
…dger Every reviewable source/config/script file (621) put through a systematic per-file verdict: 524 clean, 97 flagged. The ledger lists each file with its status; flagged fixes are in REMEDIATION_PLAN.md. Flag breakdown: 74 size/SRP (decomposition worklist), 27 broad-except (Python), 10 root-user Dockerfiles, 3 any-type (TS), 2 bare-except. The sweep surfaced two things earlier passes missed: - bare `except:` in scripts/seed_historical_metrics.py and scripts/stress_test_comprehensive.py (swallows KeyboardInterrupt) — added. - corrected the root-Dockerfile count from "6 of 8" to the true "10 of 12" (only edge-bridge + frontend run non-root). Coverage is now literal: no file is unmarked.
…tegration timeouts Phase A of the remediation plan: - JwtValidator: the fallback catch leaked e.getMessage() to the caller; now returns a generic "Invalid token" and logs the detail server-side only (OWASP information-exposure). - RefreshTokenService: replay of an already-revoked refresh token is now treated as theft — the whole token family is revoked (RFC 6819 / OAuth 2 reuse detection). Added RefreshTokenServiceTest covering reuse, valid, and unknown-token paths. - Slack/PagerDuty integrations: replaced `new RestTemplate()` (no timeout) with a SimpleClientHttpRequestFactory carrying connect+read timeouts, so a hung endpoint cannot block the notification threads (Release It).
- edge-bridge: adapter metrics were appended to b.metrics and then overwritten by the periodic device collection, so they were never uploaded. Queue them in a separate adapterMetrics slice and drain it into each upload batch. (100 Go Mistakes: shared-state clobber.) - device-protocol-c/transport_tcp.c: tcp_send spun the CPU on EAGAIN; now waits for writability via select. Check fcntl(F_GETFL) before OR-ing O_NONBLOCK (don't build on -1) and close the fd on that error path; check setsockopt. (Seacord ch.7 robust I/O.)
10 of 12 images ran as root (only edge-bridge and frontend were non-root). Added a dedicated non-root `app` user to every Java service image (alpine adduser), the ai-diagnostic image, both simulators, the 5G station, and the testing simulator (debian useradd), with USER set before the entrypoint. (CIS/Newman container hardening.) Verified: ai-diagnostic builds and starts as non-root (imports resolve; report generation writes to an in-memory buffer, not the read-only workdir). Java services only read their jar, so no write-permission risk.
…positives - scripts: replaced 3 bare `except:` (seed_historical_metrics.py, stress_test_comprehensive.py) with `except Exception:` so they no longer swallow KeyboardInterrupt/SystemExit (PEP8). - plan: on inspection, two frontend flags are false positives, marked RESOLVED — Reports.tsx is a static download page (per-action states already present, no page fetch), and the `any` hits are in comments (vite-env.d.ts, mockHelpers.ts), not real code.
CI compiled and tested but never built the images, so three Dockerfile/ runtime bugs shipped this session and were only caught by the live bring-up (edge-bridge Go 1.21-vs-1.23, ai-diagnostic PYTHONPATH). Added an Image Build job that builds the edge-bridge (Go), ai-diagnostic (Python) and monitoring-service (Java multi-stage) images — the three Dockerfile shapes that have actually broken. Uses direct `docker build` with the correct per-image context (avoids compose var interpolation). Verified all three build commands locally. (Nygard: test what you deploy.)
The cloud client retried with a fixed RetryDelay, so after a cloud outage every bridge retried in lockstep (thundering herd). Replaced with capped exponential backoff + full jitter (uniform over [0, min(base*2^n, 30s)]). Added backoff_test.go asserting the bound and the overflow clamp. (Release It: avoid retry storms.)
Removed genuinely dead code (0 references anywhere, compiles+tests green without them): - common HealthConstants.java (173 LOC, every constant unused) - base-station ConnectionProfile entity + ConnectionProfileRepository (unused entity/repo pair) Also flagged (not deleted — it is a real, complete capability, just dormant): api-gateway TokenRevocationService is a full Redis-backed JWT blacklist that is never wired into JwtValidator, so logout does not actually invalidate a token. Marked HIGH in the plan to wire it in.
device_protocol.py: the client.close() cleanup caught bare Exception; narrowed to OSError. The catch-all in the accept/handle loops is a deliberate server-robustness pattern (log + keep serving) and is left as-is. Plan updated to reflect the distinction.
…IDRs - auth JwtUtil now stamps issuer "basestation-platform"; the gateway JwtValidator requires that issuer and tolerates 30s clock skew — a token minted by a foreign issuer is rejected (OAuth2iA ch.11). Added a wrong-issuer rejection test; updated the token-builder test helpers. - JwtAuthenticationFilter: the actuator IP allow-list was String.split on every request; now parsed once (lazily cached) (Release It: hot path).
The Java images build from the repo root, so the whole tree (host target/, .git, node_modules, docs) was shipped to the daemon. Expanded the root and frontend .dockerignore and added edge-bridge and ai-diagnostic ones to exclude build output, VCS, node_modules, caches, tests and docs. Verified monitoring-service still builds from the lean context.
Only one image self-described a health check. Added a HEALTHCHECK to all five Java services hitting /actuator/health via the alpine base's busybox wget (verified present). Makes the images self-describing regardless of the orchestrator (compose already had its own healthchecks).
On close inspection during execution, several plan items are already correct or carry real risk that makes a naive fix wrong — marked rather than churned: - monitoring Optional.get(): every site is guarded by an isEmpty early return — VERIFIED SAFE. - notification async + pool: sendAsync catches+logs+failedFuture, pool is a bounded ThreadPoolTaskExecutor + CallerRunsPolicy — VERIFIED GOOD. - base-station @ElementCollection EAGER→LAZY: real N+1, BUT params are read outside the txn (controller) so a flip would throw LazyInitializationException — needs LAZY + fetch-join + integration test, not a flip. - EJ-62 status/severity: fixed-domain fields already are enums; the remaining Strings are external/parsed boundaries + frontend contract.
- CONTRIBUTING.md: per-language build/test loops, the test-first rule, commit/branch conventions, security model summary. - docs/adr/: ADRs for the three big decisions the plan flagged as undocumented — band-neutral metric model, gateway-fronted trust model, Kubernetes-DNS over Eureka. - testing/README.md: note the band-neutral NR metrics + point to /api/v1/metrics/catalog.
AlertingService inlined ~220 lines of default alert-rule literals in initializeDefaultRules(). Moved the 21-rule catalogue into a dedicated DefaultAlertRules provider (thresholds resolved from AlertThresholdConfig); the service now calls DefaultAlertRules.all(cfg).forEach(this::addRule) and holds evaluation logic. AlertingService 685 -> 468 LOC. Behaviour preserved: full monitoring suite (158 tests) green.
tmf-api was built in the reactor but not deployable (Newman: every service independently deployable). Added: - tmf-api/Dockerfile (multi-stage, non-root, HEALTHCHECK, port 8086) - docker-compose service (Mongo via discrete SPRING_DATA_MONGODB_* creds, internal secret, resource limits) - Helm values entry + templates/services/tmf-api.yaml (Deployment+Service) - Prometheus scrape target (compose + Helm) restored, now that it runs - CI image-build step for tmf-api Switched tmf-api's Mongo config from a composed URI to discrete host/username/password (consistent with the other services and the Helm secret pattern). Verified: image builds, compose config valid, helm lint + template (default and prod) render tmf-api, 84 tmf-api tests green.
TokenRevocationService (Redis blacklist) existed but was never wired, so logout did not invalidate a JWT. Now: - JwtAuthenticationFilter checks isRevoked(tokenHash, username, iat) after validation and rejects revoked tokens (per-token + user-wide revocation). Fails open on a Redis error so a store outage can't lock everyone out. - LogoutRevocationFilter (GlobalFilter) blacklists the presented token on POST /api/v1/auth/logout, then forwards to auth-service. Test added: a revoked token now returns 401. (OAuth 2 in Action ch.11.)
The notification/diagnostic queues had no dead-letter path, so a message that kept failing would be redelivered forever or dropped. Added a fanout dead-letter exchange (alerts.dlx) + queue (notifications.dlq), wired both consumer queues to it via x-dead-letter-exchange, and set the listener container to reject (not requeue) exhausted messages so they land in the DLQ for inspection/replay. (Release It: fail fast, keep a record.) Test verifies the dead-letter wiring.
…ponse The cloud client retried every non-auth error, so a metrics upload whose POST reached the server but returned an error/late response could be re-sent and double-recorded. Errors that occur after a response are now marked (errResponseReceived); doRequest retries them only for idempotent methods (GET/PUT/DELETE), never for POST/PATCH. Transport failures (no response) are still retried for any method. Tests: POST 500 hits once, GET 500 retries. (Addresses the plan's idempotency finding without cross-service infra.)
…e-config warning transport_tls.c targeted the end-of-life mbedTLS 2.x API and did not compile against 3.x. Migrated behind MBEDTLS_VERSION_MAJOR guards (2.x path preserved verbatim, zero regression): - conf_min/max_version -> conf_min/max_tls_version + MBEDTLS_SSL_VERSION_TLS1_x - pk_parse_keyfile gains the RNG parameters on 3.x - ssl_session_resumed (informational) reported as 0 on 3.x Also warn loudly when server verification is disabled (Seacord: fail loudly on insecure config). Verified against the installed mbedTLS 3.6.5: `make TLS=1` now compiles clean under -Werror. Added a CI step (installs libmbedtls-dev, builds TLS=1) so the TLS path stays covered.
MetricValueValidator had ~30 structurally identical validateXxx methods (if out-of-range -> invalid message; else valid). Replaced them with one data-driven range(min, max, label, unit, format) helper, keeping only the genuinely different rules (percentages, discrete rank, binary sensors, mixed-unit angle/direction/runtime, packet-loss). 543 -> 288 lines. Behaviour-preserving: the exhaustive switch (compile-time safety for new MetricTypes) and every error message are unchanged; the 94 characterisation tests that lock accept/reject per category stay green.
The CI runner's libmbedtls-dev is Ubuntu's mbedTLS 2.28 (LTS), while the version-detection block used 3.x-only symbols unconditionally (mbedtls_ssl_get_version_number, MBEDTLS_SSL_VERSION_TLS1_3) plus mbedtls_ssl_session_resumed, which 2.28 does not export. Switched to mbedtls_ssl_get_version(), whose string return is stable on both 2.x and 3.x, and report the informational session_resumed field as 0. The 3.x build still compiles clean locally against 3.6.5; CI now exercises the 2.28 path.
…atch The batch loop inlined the BatchMetricEntry -> MetricDataDTO mapping, pushing recordMetricsBatch past the ~50-line guideline and nesting the mapping inside the try. Extracted a private toMetricDataDTO(entry, stationId) helper. Interactions are unchanged: the controller still maps each entry and calls service.recordMetric per entry, so the band-through- batch regression test stays green. The nested request/response DTOs are kept as static member classes (Effective Java Item 24 — types used only by this controller).
…with tests DiagnosticSessionService mixed pure metric-type -> problem-code/category string mapping in with session orchestration and had no tests over that logic. Extracted the three mapping methods into a stateless DiagnosticProblemCodeMapper (@component, injected) and added 7 characterisation tests locking the AI-service code vocabulary, the software/hardware/power/network categories, case-insensitivity and the null -> UNKNOWN fallback. Behaviour-preserving: createSession delegates to the mapper with the same null handling; the redundant Objects.requireNonNull around an Optional that already had orElse(...) is dropped. All 433 monitoring-service tests pass.
…ive modules diagnostic_service.py was a ~2800-line monolith with no direct test coverage. Added 20 characterisation tests over its pure seams (data models, rule-based backend, learning engine, cloud-client mapping helpers), then extracted those seams into focused modules under the green suite: service/models.py - Problem, Solution, LearnedPattern service/learning_engine.py - LearningEngine (feedback-driven confidence) service/backends.py - AIBackend, RuleBasedBackend, OllamaBackend + RULES service/cloud_client.py - CloudClient (solution -> cloud command) diagnostic_service.py re-exports each name, so existing importers and the runtime entrypoint are unchanged. 2807 -> 2150 LOC in the monolith. All 173 ai-diagnostic tests pass (153 existing + 20 new); the learning-confidence magic numbers are now named constants (behaviour identical).
…onolith
Continues the diagnostic_service split, test-first. Added 14 characterisation
tests over the adapter layer — transport adapters via fake sockets (TCP parse->
dispatch->serialise, Serial framing) and the HTTPAdapter Flask surface via a
real test client (health, diagnose, HMAC auth 401/403/200, learning stats) —
then extracted:
service/transport_adapters.py - ProtocolAdapter, TCP/Serial/MQTT adapters
service/optional_services.py - the ~14 optional AI-subsystem imports, their
*_AVAILABLE flags and the ERR_*/ROUTE_ constants,
shared so the split avoids a circular import
service/http_adapter.py - HTTPAdapter (Flask API, ~50 routes)
diagnostic_service.py re-exports every name, so importers and the runtime
entrypoint are unchanged. The monolith is now 2807 -> 386 LOC (an 86% cut;
orchestration + main only), split across 8 cohesive modules. All 187
ai-diagnostic tests pass (153 original + 34 new characterisation tests).
…enance
PredictiveMaintenanceService mixed pure numeric logic (trend/regression, fan
failure probability + status, prediction confidence, recommendation text) into
the service and had no coverage over it. Extracted, test-first:
service/maintenance_models.py - MetricDataPoint, TrendAnalysis,
FailurePrediction, ComponentHealth
service/maintenance_analytics.py - analyze_trend + assess/calculate/confidence
+ the fan/temperature/battery/fiber
recommendation text (pure functions)
The service methods are now thin delegators (signatures unchanged, so every
call site and the singleton stay put); dataclasses are re-exported for existing
importers. Added 14 characterisation tests over the analytics (regression
direction/slope/r-squared, failure probability + TTF, health thresholds,
confidence tiers). Dropped now-dead imports (numpy, statistics, dataclass,
field). 912 -> 655 LOC; magic CV/slope cutoffs named. All 201 ai-diagnostic
tests pass.
Moved the action/status/risk enums and the HealingAction / ExecutionResult records into service/healing_models.py; self_healing.py re-exports them so importers are unchanged. Decomposed under the existing green suite — the 22 self_healing tests (models + submit/approve/cancel/stats workflow) stay green, no new tests needed. Dropped now-dead imports (dataclass, field, Enum). 889 -> 799 LOC.
Moved the self-contained Isolation Forest / Isolation Tree scorer (Liu et al., pure numpy) into service/isolation_forest.py, separate from the metric-ingestion AnomalyDetector orchestration. anomaly_detection.py re-exports both classes, so importers are unchanged. Decomposed under the existing green suite — the 16 anomaly_detection tests (which import IsolationForest directly) stay green. Randomness still flows through the shared seeded RNG for reproducibility. Dropped now-dead imports (random, get_rng/_rng). 718 -> 565 LOC.
son_functions.py had no coverage. Added 7 characterisation tests (MLB offload
scenarios, recommendation serialisation, enum vocabulary, SONEngine end-to-end
and function-filtered runs), then extracted, test-first:
service/son_models.py - function/status/priority enums + CellMetrics,
SONRecommendation
service/son_optimizers.py - MLB / MRO / CCO / EnergySaving optimizers
son_functions.py keeps the SONEngine orchestration + module API and re-exports
every name (importers unchanged). Dropped now-dead imports (numpy, random,
deque, dataclass/field, Enum, Set/Tuple, timedelta). 752 -> 273 LOC. All 208
ai-diagnostic tests pass (201 + 7 new).
drone_integration.py had no coverage. Added 9 characterisation tests over the
pure geometry (GeoPoint haversine distance incl. altitude, and the orbit /
tower-spiral / thermal-grid waypoint patterns with counts, radius, distance and
duration), then extracted, test-first:
service/drone_models.py - drone/mission/capture enums + GeoPoint,
Waypoint, FlightPath, CapturedData,
DroneState, Mission
service/flight_path_planner.py - FlightPathPlanner (pure geometry)
drone_integration.py keeps the DroneController + DroneIntegrationService
orchestration and re-exports every name (importers unchanged). Dropped now-dead
imports (numpy, json, dataclass/field, Enum, Callable, timedelta). 741 -> 320
LOC. All 217 ai-diagnostic tests pass (208 + 9 new).
…port bi_report_generator.py is predominantly matplotlib PDF rendering, but its Site Verification pass/warn/fail decision (3GPP/Huawei KPI acceptance) is business logic worth testing in isolation. Extracted the threshold comparison into a pure service/ssv_status.py; _get_ssv_status now delegates and maps the returned status to a display colour. +7 characterisation tests (higher-is-better and lower-is-better boundaries, unknown metric -> N/A). Behaviour unchanged. The rest of the report (rendering) is left as-is by design.
…eatures - ai-diagnostic/README.md: replace the stale file tree with the current module structure (diagnostic_service, predictive_maintenance, anomaly_detection, self_healing, son_functions, drone_integration and bi_report each split into models / pure-analytics / orchestration modules); list the real 224-test suite. - README.md: add the independently-deployable TMF API service (:8086); note Redis-backed token revocation + refresh-token reuse detection and the notification dead-letter queue. - docs/README.md: the C protocol now has TLS (mbedTLS 2.28/3.x, built in CI), no longer "planned"; refresh the updated date.
Audited each doc against the real controllers, config, tests and CI: - ARCHITECTURE.md: tmf-api is now deployed (was documented as "not yet wired in") — fixed the TMF section, added it to the system diagram, services table and pod count (19->20). Added the JWT token-revocation / refresh-reuse-detection and dead-letter-queue / idempotent-retry notes to Security. - API.md: documented the missing auth endpoints (refresh, validate, logout, revoke), the batch-record metrics endpoint, and a full TMF Open API section (638/639/642, reached directly on :8086, not via the gateway); added tmf-api and notification-service health checks. - SETUP.md: added tmf-api:8086 to the service-discovery list. - TESTING.md: corrected counts to the verified numbers (Java 62 classes/700+ tests, Python 224); "matrix builds" -> "parallel jobs per component". - docker-compose.yml: header said 11 services; it is now 15 default (+ profiled).
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.
Executes the high-impact phases of REMEDIATION_PLAN.md. Every change is verified (compile/test/build).
Phase A — correctness & security
JwtValidatorno longer returnse.getMessage()to callers (OWASP).RefreshTokenServiceTest.RestTemplatenow has connect+read timeouts (Release It).tcp_sendno longer spins on EAGAIN; checkedfcntl/setsockopt(Seacord).Phase B — build/deploy hardening
Phase D — polish
except:→except Exception:in scripts.Remaining (tracked in REMEDIATION_PLAN.md): oversized-module decomposition (large, module-by-module with tests), Python broad-except narrowing, EJ-62 enum conversions, ADRs.
🤖 Generated with Claude Code