Skip to content

FLX-ENG-RFC-007 — Subjective Review Checklist + Lifecycle Tooling Identification

Field Value
RFC ID FLX-ENG-RFC-007
Status Active — Week 3
Author Arun Singh, Senior Distinguished Engineer / Architect (Consulting)
Reviewers Shrikant (primary), Rahul, Raja
Scope Human-judgment code review checklist; QA/QC, deployment, and monitoring tooling demos
Parent Epics #7 — Subjective Review Checklist; #8 — Lifecycle Tooling Identification
Priority P2-MEDIUM / P2-MEDIUM
Related Issues #45 (subjective checklist), #41 (QA/QC demo), #42 (deploy demo), #43 (OTel demo), #44 (tooling recs)

TL;DR

Two complementary deliverables produced in Week 3 alongside the defect catalog. The subjective checklist (US-7.1) gives Shrikant's team a structured human-review framework. The lifecycle tooling demos (US-8.1–8.4) show the team how the tooling recommendation works end-to-end before the Week 4 handover.

1. Part A — Subjective Review Checklist

Task 1 · US-7.1 — Draft and Deliver Subjective Review Checklist (GitHub Issue #45)

Priority: P2 | Effort: 2 hrs | Owner: Arun Singh + Shrikant sign-off

Context: The 10 automated CI gates (RFC-005) check what tools can measure. The subjective checklist covers what only a human reviewer can judge: naming clarity, architecture cohesion, intent visibility, security design.

Step-by-step:

Phase 1 — Draft checklist (Arun, 1 hr):

# Flexli DMS Subjective Code Review Checklist
Version: 1.0 | Approved by: Shrikant + Arun

## Section 1: Readability & Naming (human judgment only)
- [ ] Method and variable names communicate intent without comments
- [ ] No abbreviations that require domain knowledge to decode (e.g. `scc` → `scanCodeController`)
- [ ] Boolean variable/method names use is/has/can prefix (e.g. `isParcelActive`, not `parcelActive`)
- [ ] No "magic numbers" — constants are named and documented

## Section 2: Architecture Cohesion
- [ ] Each class has a single, clearly stated responsibility
- [ ] Service methods contain business logic only — no SQL, no HTTP calls directly
- [ ] Repositories contain data access only — no business logic
- [ ] Controllers contain routing only — no business logic, no DB access

## Section 3: Error Design (complement to automated SAST)
- [ ] Error conditions are first-class — not afterthoughts
- [ ] Every exception type is meaningful and distinct (not `throw new Exception("something went wrong")`)
- [ ] Error messages contain enough context to debug without reading code
- [ ] No exception swallowing: `catch (Exception) {}` is always a review block

## Section 4: Security Design (human judgment complement to automated SAST)
- [ ] No business logic depends on client-supplied data without validation
- [ ] Sensitive data (PII, payment info) is never logged verbatim
- [ ] Authorization checks are at the service layer, not just at the controller
- [ ] No secrets or environment-specific values in code (complement to gitleaks)

## Section 5: Testability
- [ ] New code is unit-testable without mocking more than 2 collaborators
- [ ] Test names clearly state: Given_Condition_When_Action_Then_Result
- [ ] No tests that pass by asserting `Assert.IsTrue(true)` or similar
- [ ] Happy path AND at least one error path tested per public method

## Section 6: PR Quality
- [ ] PR title follows conventional commits format (feat, fix, refactor, test, docs, chore)
- [ ] PR description explains WHY, not just WHAT (the what is visible in the diff)
- [ ] PR is ≤ 400 lines of logic change (excluding generated code and migrations)
- [ ] PR includes test changes for all new logic

Phase 2 — Review with Shrikant (30 min): 1. Walk through each section with Shrikant 2. Adjust for DMS-specific patterns (e.g. mSORT-specific naming conventions) 3. Add any Flexli-specific checklist items Shrikant knows from experience 4. Agree: which items are blocking (must pass) vs. advisory (flag but don't block)?

Phase 3 — Commit and wire into PR template: 1. Commit checklist to docs/developer-guide/SUBJECTIVE-REVIEW-CHECKLIST.md 2. Create .github/PULL_REQUEST_TEMPLATE.md: 

## Summary
[One paragraph: WHY this change, not what]

## Type of change
- [ ] Bug fix
- [ ] New feature
- [ ] Refactor
- [ ] Documentation

## Subjective Review Checklist (reviewer completes)
> See [full checklist](../docs/developer-guide/SUBJECTIVE-REVIEW-CHECKLIST.md)
- [ ] Naming communicates intent
- [ ] Single responsibility maintained
- [ ] Errors are first-class
- [ ] No untested happy paths
3. Exit signal: Checklist committed; PR template active on next PR opened

2. Part B — Lifecycle Tooling Identification

Task 2 · US-8.1 — QA/QC Demo (GitHub Issue #41)

Priority: P1 | Effort: 1 hr | Owner: Arun Singh

Demo: pre-commit hooks + Semgrep scanning on a live DMS module

Step-by-step setup for demo: 1. Install pre-commit framework: 

pip install pre-commit
2. Create .pre-commit-config.yaml: 
repos:
  - repo: https://github.com/gitleaks/gitleaks
    rev: v8.18.0
    hooks:
      - id: gitleaks
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.5.0
    hooks:
      - id: trailing-whitespace
      - id: end-of-file-fixer
      - id: check-merge-conflict
  - repo: local
    hooks:
      - id: dotnet-format
        name: dotnet format
        entry: dotnet format
        language: system
        types: [c#]
        pass_filenames: false
3. Run pre-commit install → hooks activated on git commit 4. Live demo sequence: 
# Show: intentionally add trailing whitespace to InfeedController.cs
echo "// test " >> src/.../InfeedController.cs
git add .
git commit -m "demo: test pre-commit hooks"
# Expected: pre-commit hook fires, rejects commit, shows issue
5. Run Semgrep live on DMS: 
semgrep --config "p/csharp" src/distribution-management-server-layered/ \
  --severity ERROR --verbose
6. Show findings in VS Code via Semgrep extension 7. Exit signal: Pre-commit hooks installed and working; live demo recorded or screenshots committed

Task 3 · US-8.2 — Deployment Demo (GitHub Issue #42)

Priority: P1 | Effort: 1 hr | Owner: Arun Singh

Demo: CI gate template enforcing metrics framework on a sample PR

Step-by-step: 1. Create a sample feature/demo-ci-gate-enforcement branch 2. Intentionally introduce a SAST finding (e.g. use string.Format with user input in a SQL-adjacent context) 3. Open PR to main 4. Show: GitHub Actions CI runs → sast job fails → PR cannot be merged 5. Fix the finding → CI goes green → PR can be merged 6. Walk through the build.yml file explaining each gate 7. Show CodePulse updating with the new PR event 8. Exit signal: Demo PR opened, CI run screenshot captured, committed to docs/demos/deployment-demo-YYYY-MM-DD.md

Task 4 · US-8.3 — Monitoring Demo (GitHub Issue #43)

Priority: P1 | Effort: 1.5 hrs | Owner: Arun Singh

Demo: OpenTelemetry instrumentation walkthrough on DMS

Note: This demo previews the work to be fully implemented in RFC-PoC-5.1 (#61). In Week 3, it is a walkthrough demo, not a full production implementation.

Step-by-step for demo: 1. Start the local Grafana + Loki + Prometheus stack: 

docker compose -f docker-compose.observability.yml up -d
2. Start DMS locally with OTel endpoint: 
OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317 \
dotnet run --project src/distribution-management-server-layered/
3. Send a few test requests: 
curl -X POST http://localhost:5000/api/infeed -H "Content-Type: application/json" -d '{...}'
4. Open Grafana at http://localhost:3000 5. Show: - Metrics panel: request rate, error rate, latency P99 - Logs panel: structured logs with correlation ID - Traces panel: distributed trace from HTTP request → DB query - tenantId label on all signals (preview of RFC-PoC-5.1 multi-tenant feature) 6. Explain: "In full RFC PoC implementation, each tenant gets their own filtered view" 7. Exit signal: Grafana screenshot showing DMS metrics committed to docs/demos/monitoring-demo-YYYY-MM-DD.md

Task 5 · US-8.4 — Tooling Recommendations Document (GitHub Issue #44)

Priority: P1 | Effort: 1.5 hrs | Owner: Arun Singh

This document is the final Week 3 output for the lifecycle tooling epic.

Step-by-step: 1. Compile tooling recommendations based on Weeks 1–3 findings:

# DMS Recommended Tooling Stack
Author: Arun Singh | Date: YYYY-MM-DD | Version: 1.0

## QA/QC Tooling

| Category | Tool | License | Rationale |
|----------|------|---------|-----------|
| Pre-commit | pre-commit framework | MIT | Catches format, secrets, whitespace before commit |
| SAST | Semgrep OSS | LGPL | C# + OWASP rules; SARIF output to GitHub Security |
| Secret scan | gitleaks | MIT | Fastest secret detection; GitHub Actions native |
| Format | dotnet format | MIT | Built into .NET SDK; zero additional install |
| Complexity | lizard | MIT | Language-agnostic; CI-friendly threshold enforcement |

## Deployment Tooling

| Category | Tool | License | Rationale |
|----------|------|---------|-----------|
| CI/CD | GitHub Actions | Free for public/OS | Already in org; no additional cost |
| Container | Docker + GHCR | Free (org limit) | OCI standard; no registry cost for org packages |
| Signing | Cosign (keyless) | Apache 2.0 | No key management; OIDC-based; SLSA compliant |
| SBOM | CycloneDX .NET | Apache 2.0 | .NET native; JSON + XML output |

## Monitoring Tooling

| Category | Tool | License | Rationale |
|----------|------|---------|-----------|
| Metrics | Prometheus | Apache 2.0 | Industry standard; integrates with Grafana |
| Dashboards | Grafana OSS | AGPL | Free self-hosted; pre-built OTel dashboards |
| Logs | Loki | AGPL | Prometheus-compatible; low cost self-hosted |
| Traces | Tempo | AGPL | Native OTel; Grafana integrated |
| Instrumentation | OTel .NET SDK | Apache 2.0 | CNCF standard; vendor-neutral |
| DORA SaaS | CodePulse | $149/month | Phase 1 — buy vs build |

## Phase 2 Trigger

When: Change Lead Time > 7 days for 2 consecutive weeks OR CodePulse cost becomes prohibitive.  
Action: Self-host DORA middleware on Hetzner via Terraform (see US-4.4 #49 for Terraform handover).
  1. Commit document to docs/developer-guide/TOOLING-RECOMMENDATIONS.md
  2. Link from docs/index.md
  3. Exit signal: Document committed; Raja acknowledges receipt in GitHub issue comment

3. Dependencies

Dependency Required for
Week 1 scans complete (#16) Task 5 (tooling recs informed by scan results)
OTel observability stack started (RFC-PoC-5.1 #61) Task 4 (monitoring demo)
CI gates configured (RFC-005) Task 3 (deployment demo uses CI gate enforcement)

4. Success Criteria

Subjective Review: - [ ] Checklist committed to docs/developer-guide/SUBJECTIVE-REVIEW-CHECKLIST.md - [ ] PR template created at .github/PULL_REQUEST_TEMPLATE.md - [ ] Shrikant has reviewed and approved checklist

Lifecycle Tooling: - [ ] Pre-commit hooks demo executed; screenshots committed - [ ] Deployment demo PR opened, CI failure + fix demonstrated - [ ] Grafana monitoring demo screenshot committed - [ ] Tooling recommendations document committed and linked from index