Skip to content

feat(compliance): map findings to SOC2/PCI/ISO controls (--compliance) - #109

Open
affaan-m wants to merge 1 commit into
mainfrom
feat/compliance-control-mapping
Open

feat(compliance): map findings to SOC2/PCI/ISO controls (--compliance)#109
affaan-m wants to merge 1 commit into
mainfrom
feat/compliance-control-mapping

Conversation

@affaan-m

@affaan-m affaan-m commented Jun 23, 2026

Copy link
Copy Markdown
Owner

Adds the auditor-ready compliance artifact GRC teams hand to auditors — a high-margin Enterprise upsell. Maps findings to audit-framework control IDs instead of leaving buyers with a raw findings list.

What

agentshield scan --compliance soc2       # SOC 2 Trust Services Criteria
agentshield scan --compliance pci,iso    # comma-separated
agentshield scan --compliance all        # SOC 2 + PCI DSS + ISO 27001

Each framework prints a control-coverage table (control id, title, highest severity, finding count, example findings), ordered by severity:

## Compliance Mapping: SOC 2 (Trust Services Criteria)
Mapped 194/194 findings to 5 control(s). 0 finding(s) had no mapped control.

| Control | Title                        | Highest  | Findings | Examples |
| CC6.1   | Logical access - credentials | critical | 76       | Hardcoded Anthropic API key; ... |

Implementation

  • src/compliance/index.tsCONTROL_MAP (finding category → control IDs for SOC 2 / PCI DSS v4.0 / ISO 27001:2022 Annex A), mapFindingsToControls (aggregates per control, tracks highest severity + up to 3 example titles), parseFrameworks (soc2/pci/iso/all, comma-separated, ignores unknown tokens), renderComplianceReport (markdown table).
  • CLI — scan --compliance <frameworks> emits a section per framework through the existing auxiliary-output channel; exits 1 on an unknown framework.
  • README — Compliance Mapping section + CLI Reference entry.

The mapping is finding-category-level guidance, not a certified crosswalk — stated in the rendered report and the docs; confirm control applicability with your auditor.

Verification

  • 10 new tests (per-framework control mapping, aggregation + highest-severity, mapped/unmapped counts, severity ordering, example cap, framework parsing, rendering + empty set).
  • Full suite green (1873 tests), tsc --noEmit + eslint clean.
  • CLI smoke: SOC 2 + PCI render against the vulnerable example; unknown framework exits 1 with a clear error.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a --compliance <frameworks> CLI option to map security findings to compliance framework controls (SOC2, PCI, ISO, or all)
    • Compliance reports display control coverage with severity aggregation and example findings; includes guidance disclaimer
  • Documentation

    • Added documentation for the new compliance flag with example usage patterns
  • Tests

    • Added comprehensive test coverage for compliance mapping functionality

Greptile Summary

This PR adds a --compliance flag to agentshield scan that maps findings to SOC 2, PCI DSS v4.0, and ISO 27001:2022 control IDs, rendering an auditor-ready markdown coverage table per framework.

  • src/compliance/index.ts introduces CONTROL_MAP (all 10 FindingCategory values × 3 frameworks), mapFindingsToControls (aggregates per-control severity/count/examples), parseFrameworks (handles all and comma-separated input), and renderComplianceReport (markdown table with disclaimer).
  • src/index.ts wires the option in after the main report is emitted; exits 1 only when every supplied token is unrecognised.
  • 10 new tests cover per-framework mapping, severity aggregation, example capping, and rendering — but no test covers finding titles that contain a pipe character, which would corrupt the markdown table.

Confidence Score: 3/5

Safe to merge after fixing pipe-escaping in the markdown table renderer; the rest of the change is additive and well-tested.

The compliance renderer inserts finding.title directly into a pipe-delimited markdown table. Any title that contains a | character — plausible for rules that echo back shell commands or matched patterns from scanned files — will corrupt every row from that point on in the rendered table. The feature is new and opt-in so it does not regress existing scans, but the output contract it advertises (auditor-ready markdown artifact) can be silently broken by real-world input.

src/compliance/index.ts — specifically renderComplianceReport, where examples, control.controlTitle, and control.controlId are interpolated into table cells without escaping pipes.

Important Files Changed

Filename Overview
src/compliance/index.ts New compliance module implementing control mapping, aggregation, and markdown rendering. Logic is correct for known categories and type-safety is well enforced, but finding titles are interpolated raw into the markdown table without pipe-escaping, which corrupts output when any title contains `
src/index.ts Wires --compliance option into the scan command. Error handling for all-unknown input is correct; silent partial-parse for mixed valid/invalid tokens is a UX concern but not a crash risk.
tests/compliance/compliance.test.ts 10 tests covering per-framework mapping, severity aggregation, mapped/unmapped counts, severity ordering, example cap, framework parsing, rendering, and empty-set handling. No table-formatting edge cases (pipe in titles) are tested.
README.md Adds Compliance Mapping section and CLI reference entry. Documentation matches implementation, disclaimer about non-certified crosswalk is present.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[agentshield scan --compliance value] --> B[parseFrameworks]
    B --> C{frameworks.length === 0?}
    C -- yes --> D[console.error + process.exit 1]
    C -- no --> E[for each framework]
    E --> F[mapFindingsToControls
findings x CONTROL_MAP]
    F --> G[aggregate per controlId
count · severity · examples]
    G --> H[sort by highestSeverity
then findingCount]
    H --> I[renderComplianceReport
markdown table]
    I --> J[writeAuxiliaryOutput]
    J --> E
    E --> K[logger.log compliance info]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[agentshield scan --compliance value] --> B[parseFrameworks]
    B --> C{frameworks.length === 0?}
    C -- yes --> D[console.error + process.exit 1]
    C -- no --> E[for each framework]
    E --> F[mapFindingsToControls
findings x CONTROL_MAP]
    F --> G[aggregate per controlId
count · severity · examples]
    G --> H[sort by highestSeverity
then findingCount]
    H --> I[renderComplianceReport
markdown table]
    I --> J[writeAuxiliaryOutput]
    J --> E
    E --> K[logger.log compliance info]
Loading

Reviews (1): Last reviewed commit: "feat(compliance): map findings to SOC2/P..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Auditor-ready coverage artifact for GRC teams: maps each finding (by category)
to control IDs across SOC 2 Trust Services Criteria, PCI DSS v4.0, and ISO/IEC
27001:2022 Annex A, then renders a control-coverage table (control, title,
highest severity, finding count, examples) ordered by severity.

- src/compliance/index.ts: CONTROL_MAP (category -> controls per framework),
  mapFindingsToControls (aggregates findings per control, tracks highest
  severity + examples), parseFrameworks (soc2/pci/iso/all, comma-separated),
  renderComplianceReport (markdown table + 'guidance, not certified crosswalk'
  disclaimer).
- CLI: scan --compliance <frameworks> emits a mapping section per framework via
  the existing auxiliary-output channel; exits 1 on an unknown framework.
- README: Compliance Mapping section + CLI Reference entry.

10 new tests. Full suite 1873 green, tsc + eslint clean, CLI smoke verified
(SOC2+PCI render, unknown framework exits 1). High-margin Enterprise upsell.
@ecc-tools

ecc-tools Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key: "tools"
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
📝 Walkthrough

Walkthrough

A new src/compliance/index.ts module maps scan Finding objects to SOC2, PCI, and ISO control IDs via a static CONTROL_MAP. Three functions are exported: mapFindingsToControls, parseFrameworks, and renderComplianceReport. The scan CLI command gains a --compliance <frameworks> option that invokes this module post-scan. Tests and README documentation are added.

Changes

Compliance Mapping Feature

Layer / File(s) Summary
Compliance types, frameworks, and CONTROL_MAP
src/compliance/index.ts
Defines ComplianceFramework type, COMPLIANCE_FRAMEWORKS, FRAMEWORK_NAMES, CONTROL_MAP keyed by FindingCategory, and the ControlCoverage/ComplianceReport public interfaces.
mapFindingsToControls, parseFrameworks, renderComplianceReport, and tests
src/compliance/index.ts, tests/compliance/compliance.test.ts
Implements finding aggregation into sorted per-control coverage, comma-separated/"all" framework parsing, and markdown table rendering; full Vitest suite validates mapping, severity ranking, totals, ordering, and rendering edge cases.
CLI option registration and --compliance execution path
src/index.ts
Imports compliance utilities, registers --compliance <frameworks> on the scan command, and implements the post-scan path that validates frameworks, maps filteredResult.findings, outputs each report, and logs mapped control counts.
README documentation
README.md
Adds "Compliance Mapping" section with usage examples, a sample coverage table, a guidance disclaimer, and the --compliance <frameworks> entry in the CLI Reference flag list.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main addition—a new compliance feature that maps findings to SOC2/PCI/ISO controls with CLI flag support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/compliance-control-mapping

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread src/compliance/index.ts
Comment on lines +192 to +196
if (report.controls.length === 0) {
lines.push("No findings mapped to controls for this framework.");
lines.push("");
} else {
lines.push("| Control | Title | Highest | Findings | Examples |");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Pipe characters in finding titles corrupt the markdown table

finding.title is inserted directly into a |-delimited markdown table row without any sanitization. If a title contains a literal pipe — e.g., a rule that captures a shell command like bash -c "cmd1 | cmd2" — the rendered table gains extra columns and most markdown renderers will mangle every subsequent row in that table. The examples cell is the most exposed site because multiple titles are joined, but control.controlId and control.controlTitle are also inserted raw.

Pipe characters should be escaped (replace | with \|) before interpolation into any table cell.

Comment thread src/compliance/index.ts
Comment on lines +162 to +169
frameworkName: FRAMEWORK_NAMES[framework],
totalFindings: findings.length,
mappedFindingCount,
unmappedFindingCount: findings.length - mappedFindingCount,
controls,
};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unknown tokens in a comma-separated list are silently dropped

parseFrameworks("soc2,hipaa") returns ["soc2"] — the unknown token is silently filtered out. The CLI only exits 1 when the entire result is empty (frameworks.length === 0), so a user who mistypes one framework in a comma list gets a partial report with no indication that anything was ignored. The PR description states "exits 1 on an unknown framework," but that is only true when every token is unrecognised. The test name ("ignores unknown tokens") confirms this is intentional, but users scanning for soc2,iso who accidentally write soc2,is0 (zero) will silently get only the SOC 2 report.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Around line 417-425: The fenced code block containing the compliance mapping
table is missing a language identifier which violates the MD040 linting rule.
Locate the code block that starts with the triple backticks before the "##
Compliance Mapping: SOC 2 (Trust Services Criteria)" heading and add "text" as
the language identifier immediately after the opening backticks (change ``` to
```text).

In `@src/compliance/index.ts`:
- Around line 104-112: The MutableCoverage interface violates immutability rules
for src/ files by using mutable fields and mutable array types. Convert all
fields in the MutableCoverage interface to be readonly by adding the readonly
modifier before each field declaration, and change the exampleFindings field
type from string[] to ReadonlyArray<string>. Additionally, review the code
sections at lines 122-151 and 183-205 that also apply these same violations and
update any mutation operations (such as += or push operations) to use immutable
alternatives like concatenation or array spreading instead of in-place
mutations.
- Around line 89-95: The SEVERITY_RANK constant in the compliance module is
duplicating severity ordering information that already exists as SEVERITY_ORDER
in src/types.ts, creating a maintenance issue where the two can drift apart.
Remove the local SEVERITY_RANK constant definition and import SEVERITY_ORDER
from src/types.ts instead, then update any code using SEVERITY_RANK to use the
imported SEVERITY_ORDER or derive the ranking map from it to maintain a single
source of truth for severity ordering.
- Around line 170-177: The parseFrameworks function currently returns duplicate
frameworks when the input contains duplicate values (e.g., "soc2,soc2"). Add
deduplication logic to remove duplicate framework values before returning the
result. This can be accomplished by converting the filtered frameworks array
into a Set to remove duplicates, then converting back to an array while
maintaining the ReadonlyArray<ComplianceFramework> return type. Apply this
deduplication to both the "all" case and the parsed comma-separated case.
- Around line 199-202: The controlTitle and exampleFindings values are being
injected directly into the markdown table row without escaping special
characters, which can break table formatting if they contain pipe characters or
newlines. Before constructing the table row string where controlTitle and
examples variables are used, escape pipe characters and newline characters in
both values to ensure they don't break the markdown table structure. This
escaping should happen after the examples variable is created from the join
operation but before the table row string is constructed and pushed to the lines
array.

In `@src/index.ts`:
- Around line 415-419: The parseFrameworks() function silently filters out
unknown framework tokens, so input like "--compliance soc2,bogus" succeeds with
only "soc2" instead of alerting the user to the typo. Implement strict
validation using Zod at the CLI boundary (before calling parseFrameworks()) to
ensure all tokens in options.compliance are recognized frameworks. The
validation should reject the entire input if any unknown tokens are detected,
rather than allowing partial success. This way, users are immediately notified
of any mistakes in their compliance framework specifications.

In `@tests/compliance/compliance.test.ts`:
- Around line 10-20: Replace the local fixture function `f()` with the
centralized test helper factory `makeFinding()` throughout the
compliance.test.ts file. Remove the ad-hoc `f()` function definition and update
all calls to `f(overrides)` to use `makeFinding(overrides)` instead. This
ensures test data creation is consistent across the codebase and maintainable
from a central location. Import `makeFinding()` from the shared test factories
if not already imported.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7ca20624-e365-4006-9b7d-ab00571d6dfc

📥 Commits

Reviewing files that changed from the base of the PR and between 25d91f0 and 89d60b5.

📒 Files selected for processing (4)
  • README.md
  • src/compliance/index.ts
  • src/index.ts
  • tests/compliance/compliance.test.ts

Comment thread README.md
Comment on lines +417 to +425
```
## Compliance Mapping: SOC 2 (Trust Services Criteria)

Mapped 194/194 findings to 5 control(s). 0 finding(s) had no mapped control.

| Control | Title | Highest | Findings | Examples |
| CC6.1 | Logical access - credentials | critical | 76 | Hardcoded Anthropic API key; ... |
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a language identifier to the fenced code block.

This block is missing a fence language (MD040).

📝 Proposed fix
-```
+```text
 ## Compliance Mapping: SOC 2 (Trust Services Criteria)
@@
-| CC6.1   | Logical access - credentials | critical | 76       | Hardcoded Anthropic API key; ... |
+| CC6.1   | Logical access - credentials | critical | 76       | Hardcoded Anthropic API key; ... |
</details>

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion

🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 417-417: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` around lines 417 - 425, The fenced code block containing the
compliance mapping table is missing a language identifier which violates the
MD040 linting rule. Locate the code block that starts with the triple backticks
before the "## Compliance Mapping: SOC 2 (Trust Services Criteria)" heading and
add "text" as the language identifier immediately after the opening backticks
(change ``` to ```text).

Source: Linters/SAST tools

Comment thread src/compliance/index.ts
Comment on lines +89 to +95
const SEVERITY_RANK: Readonly<Record<Severity, number>> = {
critical: 0,
high: 1,
medium: 2,
low: 3,
info: 4,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Avoid duplicating severity order constants.

SEVERITY_RANK duplicates SEVERITY_ORDER from src/types.ts; this can drift and silently reorder control output later.

♻️ Proposed fix
-import type { Finding, FindingCategory, Severity } from "../types.js";
+import { SEVERITY_ORDER, type Finding, type FindingCategory, type Severity } from "../types.js";
@@
-const SEVERITY_RANK: Readonly<Record<Severity, number>> = {
-  critical: 0,
-  high: 1,
-  medium: 2,
-  low: 3,
-  info: 4,
-};
+const SEVERITY_RANK = SEVERITY_ORDER;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const SEVERITY_RANK: Readonly<Record<Severity, number>> = {
critical: 0,
high: 1,
medium: 2,
low: 3,
info: 4,
};
import { SEVERITY_ORDER, type Finding, type FindingCategory, type Severity } from "../types.js";
const SEVERITY_RANK = SEVERITY_ORDER;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/compliance/index.ts` around lines 89 - 95, The SEVERITY_RANK constant in
the compliance module is duplicating severity ordering information that already
exists as SEVERITY_ORDER in src/types.ts, creating a maintenance issue where the
two can drift apart. Remove the local SEVERITY_RANK constant definition and
import SEVERITY_ORDER from src/types.ts instead, then update any code using
SEVERITY_RANK to use the imported SEVERITY_ORDER or derive the ranking map from
it to maintain a single source of truth for severity ordering.

Comment thread src/compliance/index.ts
Comment on lines +104 to +112
interface MutableCoverage {
framework: ComplianceFramework;
controlId: string;
controlTitle: string;
findingCount: number;
severities: Record<Severity, number>;
highestSeverity: Severity;
exampleFindings: string[];
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

This implementation breaks the src/**/*.ts immutability rules.

MutableCoverage uses mutable fields, arrays are typed as mutable (string[]), and state is mutated via +=/push. This conflicts with the repository’s explicit immutability constraints for src/.

As per coding guidelines, src/**/*.ts: "All arrays must be typed as ReadonlyArray and all interfaces must use readonly fields for immutability" and "No mutation, no any types, and no console.log in src/ files".

Also applies to: 122-151, 183-205

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/compliance/index.ts` around lines 104 - 112, The MutableCoverage
interface violates immutability rules for src/ files by using mutable fields and
mutable array types. Convert all fields in the MutableCoverage interface to be
readonly by adding the readonly modifier before each field declaration, and
change the exampleFindings field type from string[] to ReadonlyArray<string>.
Additionally, review the code sections at lines 122-151 and 183-205 that also
apply these same violations and update any mutation operations (such as += or
push operations) to use immutable alternatives like concatenation or array
spreading instead of in-place mutations.

Source: Coding guidelines

Comment thread src/compliance/index.ts
Comment on lines +170 to +177
export function parseFrameworks(value: string): ReadonlyArray<ComplianceFramework> {
const normalized = value.trim().toLowerCase();
if (normalized === "all") return COMPLIANCE_FRAMEWORKS;
return normalized
.split(",")
.map((v) => v.trim())
.filter((v): v is ComplianceFramework => (COMPLIANCE_FRAMEWORKS as ReadonlyArray<string>).includes(v));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Deduplicate parsed frameworks before returning.

Inputs like soc2,soc2 currently return duplicates, which leads to duplicate report sections and duplicated log lines.

🐛 Proposed fix
 export function parseFrameworks(value: string): ReadonlyArray<ComplianceFramework> {
   const normalized = value.trim().toLowerCase();
   if (normalized === "all") return COMPLIANCE_FRAMEWORKS;
-  return normalized
+  return [...new Set(
+    normalized
     .split(",")
     .map((v) => v.trim())
-    .filter((v): v is ComplianceFramework => (COMPLIANCE_FRAMEWORKS as ReadonlyArray<string>).includes(v));
+    .filter((v): v is ComplianceFramework => (COMPLIANCE_FRAMEWORKS as ReadonlyArray<string>).includes(v))
+  )];
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function parseFrameworks(value: string): ReadonlyArray<ComplianceFramework> {
const normalized = value.trim().toLowerCase();
if (normalized === "all") return COMPLIANCE_FRAMEWORKS;
return normalized
.split(",")
.map((v) => v.trim())
.filter((v): v is ComplianceFramework => (COMPLIANCE_FRAMEWORKS as ReadonlyArray<string>).includes(v));
}
export function parseFrameworks(value: string): ReadonlyArray<ComplianceFramework> {
const normalized = value.trim().toLowerCase();
if (normalized === "all") return COMPLIANCE_FRAMEWORKS;
return [...new Set(
normalized
.split(",")
.map((v) => v.trim())
.filter((v): v is ComplianceFramework => (COMPLIANCE_FRAMEWORKS as ReadonlyArray<string>).includes(v))
)];
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/compliance/index.ts` around lines 170 - 177, The parseFrameworks function
currently returns duplicate frameworks when the input contains duplicate values
(e.g., "soc2,soc2"). Add deduplication logic to remove duplicate framework
values before returning the result. This can be accomplished by converting the
filtered frameworks array into a Set to remove duplicates, then converting back
to an array while maintaining the ReadonlyArray<ComplianceFramework> return
type. Apply this deduplication to both the "all" case and the parsed
comma-separated case.

Comment thread src/compliance/index.ts
Comment on lines +199 to +202
const examples = control.exampleFindings.join("; ") || "-";
lines.push(
`| ${control.controlId} | ${control.controlTitle} | ${control.highestSeverity} | ${control.findingCount} | ${examples} |`
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Escape markdown cell content before table rendering.

controlTitle and exampleFindings are injected raw into table cells. | or newline characters in titles/examples can break table structure.

🛡️ Proposed fix
+const escapeMarkdownCell = (value: string): string =>
+  value.replaceAll("|", "\\|").replaceAll("\n", " ").replaceAll("\r", " ");
@@
-      const examples = control.exampleFindings.join("; ") || "-";
+      const examples = escapeMarkdownCell(control.exampleFindings.join("; ") || "-");
       lines.push(
-        `| ${control.controlId} | ${control.controlTitle} | ${control.highestSeverity} | ${control.findingCount} | ${examples} |`
+        `| ${escapeMarkdownCell(control.controlId)} | ${escapeMarkdownCell(control.controlTitle)} | ${control.highestSeverity} | ${control.findingCount} | ${examples} |`
       );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/compliance/index.ts` around lines 199 - 202, The controlTitle and
exampleFindings values are being injected directly into the markdown table row
without escaping special characters, which can break table formatting if they
contain pipe characters or newlines. Before constructing the table row string
where controlTitle and examples variables are used, escape pipe characters and
newline characters in both values to ensure they don't break the markdown table
structure. This escaping should happen after the examples variable is created
from the join operation but before the table row string is constructed and
pushed to the lines array.

Comment thread src/index.ts
Comment on lines +415 to +419
const frameworks = parseFrameworks(options.compliance);
if (frameworks.length === 0) {
console.error(`Error: --compliance expects soc2, pci, iso, or all (got "${options.compliance}")`);
process.exit(1);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read-only verification of current behavior contracts and parser wiring.
sed -n '414,430p' src/index.ts
sed -n '170,177p' src/compliance/index.ts
sed -n '94,100p' tests/compliance/compliance.test.ts

Repository: affaan-m/agentshield

Length of output: 1519


--compliance validation silently ignores unknown framework tokens.

With the current filtering logic in parseFrameworks(), input like --compliance soc2,bogus succeeds by silently dropping "bogus" and proceeding with only "soc2". The CLI validation only fails when all tokens are unrecognized. For a security scanner, this risks incomplete compliance checks due to user typos without alerting them to the mistake.

Implement strict validation that rejects any unknown framework tokens at the CLI boundary. Per coding guidelines (src/**/*.ts): "Use Zod for config validation at boundaries" and "Focus reviews on security-scanner correctness ... and preserving the CLI ... public behavior."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/index.ts` around lines 415 - 419, The parseFrameworks() function silently
filters out unknown framework tokens, so input like "--compliance soc2,bogus"
succeeds with only "soc2" instead of alerting the user to the typo. Implement
strict validation using Zod at the CLI boundary (before calling
parseFrameworks()) to ensure all tokens in options.compliance are recognized
frameworks. The validation should reject the entire input if any unknown tokens
are detected, rather than allowing partial success. This way, users are
immediately notified of any mistakes in their compliance framework
specifications.

Sources: Coding guidelines, Path instructions

Comment on lines +10 to +20
function f(overrides: Partial<Finding>): Finding {
return {
id: "x",
severity: "high",
category: "secrets",
title: "A finding",
description: "d",
file: "CLAUDE.md",
...overrides,
};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use shared test factories instead of ad-hoc f() fixtures.

Please switch this local fixture builder to the repository helper factories (makeFinding()/makeSettings()) so tests stay consistent and centrally maintainable.

As per coding guidelines, tests/**/*.test.ts: "Tests should use helper factories like makeFinding() and makeSettings() for test data creation".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/compliance/compliance.test.ts` around lines 10 - 20, Replace the local
fixture function `f()` with the centralized test helper factory `makeFinding()`
throughout the compliance.test.ts file. Remove the ad-hoc `f()` function
definition and update all calls to `f(overrides)` to use
`makeFinding(overrides)` instead. This ensures test data creation is consistent
across the codebase and maintainable from a central location. Import
`makeFinding()` from the shared test factories if not already imported.

Source: Coding guidelines

Copy link
Copy Markdown

Local review recommendation: request changes before merge. The compliance mapping output needs escaping for Markdown table cells; unescaped pipe characters in control/objective text can corrupt generated evidence tables and make downstream compliance artifacts misleading. Please add a regression covering mapped text with | characters before landing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants