Skip to content

feat(scan-soul): add --explain option for 9-domain governance model (#163)#166

Merged
thebenignhacker merged 2 commits intomainfrom
fix/scan-soul-explain-163
Apr 29, 2026
Merged

feat(scan-soul): add --explain option for 9-domain governance model (#163)#166
thebenignhacker merged 2 commits intomainfrom
fix/scan-soul-explain-163

Conversation

@thebenignhacker
Copy link
Copy Markdown
Contributor

Closes #163.

Summary

The concept-explainer text in src/ui/concept-explainers.ts:46 shipped in 0.22.0 (PR #142) cited:

Run `scan-soul --explain` for the 9-domain model.

But --explain was never registered as a Commander option, so every credential-finding scan that triggered the SOUL-governance explainer pointed users at a dead-end command.

Implementation

  • New src/soul/governance-model.ts builds a 9-domain summary from the existing CONTROL_DEFS / DOMAIN_ORDER / PROFILE_DOMAINS exports in scanner.ts — changes to the source of truth flow through automatically.
  • scan-soul Commander definition registers --explain. When set, the action handler dispatches to printGovernanceModel() and returns 0 before any scan I/O.
  • New __tests__/ui/concept-explainer-command-references.test.ts walks the concept-explainer registry and asserts every cited command/flag — both hackmyagent <verb> form and bare \ --`` inside backticks — parses via the live Commander program.

Why the cross-ref test matters

PR #142 shipped a non-existent command in user-facing copy and survived 11+ pre-push reviews because the failure mode — text rendered correctly in scan output — looked indistinguishable from a working command in code review. The CI test gates future drift: any new explainer entry citing an unregistered command/flag fails CI.

Mutation-tested before commit: replacing --explain with --bogusflag in the explainer body causes the test to fail with \hackmyagent scan-soul --bogusflag` cited but `--bogusflag` is not registered on `scan-soul``. Reverting fixes it.

Test scope is intentionally narrow (concept-explainers.ts only). Broader cross-ref of finding fix: strings, audit: blocks, and JSDoc Usage: examples will land in a follow-up — pre-existing stale-string drift in oasb-1.ts, asff.ts, opt-in.ts predates #163 and entangles with this gate.

Verification

  • npm test — 2019/2045 pass (was 2016/2042 baseline; +3 cross-ref tests).
  • npm run release-smoke:corpus — 12/12.
  • node dist/cli.js scan-soul --explain — prints 9 domains × 72 controls with correct profile→domain mapping, exits 0, runs from any cwd with no scan side-effects.
  • HMA self-scan: 84/100, same finding shape as 0.22.0 (zero new findings introduced).

Test plan

  • npm test green
  • npm run release-smoke:corpus 12/12
  • Mutation test: bogus flag injected → cross-ref test fails; revert → green
  • scan-soul --explain from clean tmpdir prints model with no scan side-effects
  • Cross-analyzer consistency unchanged (no detection logic touched)

Related

Bundled with #162 / #164 fixes for the 0.22.1 release.

scan-soul concept-explainer text in src/ui/concept-explainers.ts:46
shipped in 0.22.0 cited "Run \`scan-soul --explain\` for the 9-domain
model" but --explain was never registered as a Commander option, so
every credential-finding scan that triggered the SOUL-governance
explainer pointed users at a dead-end command.

Implementation:
- New src/soul/governance-model.ts builds a 9-domain summary from the
  existing CONTROL_DEFS / DOMAIN_ORDER / PROFILE_DOMAINS exports —
  changes to scanner.ts flow through automatically.
- scan-soul Commander definition registers --explain; when set, dispatch
  to printGovernanceModel() and exit 0 before any scan I/O.
- New __tests__/ui/concept-explainer-command-references.test.ts walks
  src/ui/concept-explainers.ts, extracts every cited command/flag
  (both \`hackmyagent <verb>\` form and bare \`<verb> --<flag>\` inside
  backticks), and asserts each parses via the live Commander program.

The cross-ref test gates future drift: any new explainer entry citing
an unregistered command/flag fails CI, regardless of whether the author
remembers to add the missing option.

Tests: 2019/2045 pass (was 2016/2042 baseline; +3 new cross-ref tests).
Smoke corpus: 12/12.
@thebenignhacker thebenignhacker enabled auto-merge (squash) April 29, 2026 20:34
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

Security Review: PR #166

VERIFICATION MANDATE CHECKLIST

Test File (__tests__/ui/concept-explainer-command-references.test.ts)

Line 100: execFileSync with untrusted input?

  • VERIFIED SAFE: Arguments are hardcoded (process.execPath, CLI_PATH, verb, --help)
  • ✅ Verb comes from parsing help output via regex (controlled source)
  • ✅ Uses execFileSync (array form) NOT shell interpolation
  • ✅ Timeout enforced (15000ms)

Line 112: Second execFileSync call

  • VERIFIED SAFE: Same pattern, hardcoded arguments only

Line 21-22: Path construction with user input?

  • VERIFIED SAFE: __dirname is internal, REPO_ROOT uses path.resolve
  • ✅ CLI_PATH/EXPLAINERS_PATH join against resolved base (no traversal vector)

Line 146: readFileSync on EXPLAINERS_PATH

  • VERIFIED SAFE: Path is hardcoded relative to repo root

ReDoS in regex patterns?

  • Line 55: /\bhackmyagent\s+([a-z][a-z0-9-]+)(?![a-z0-9-])/g — ✅ Linear (no nested quantifiers)
  • Line 68: /([a-z][a-z0-9-]+)(\s+--[a-z][a-z0-9- =<>"'/.]*)?/g — ✅ Linear (* not nested)
  • Line 85: /(?:^|\s)(--[a-z][a-z0-9-]+)\b/g — ✅ Linear
  • Line 94: /\x1b\[[0-9;]*m/g — ✅ Linear (ANSI strip pattern)
  • Line 169: Dynamically constructed regex with flag.replace(/-/g, '\\-') — ✅ Sanitized (only escapes hyphens, no injection)

CLI Changes (src/cli.ts)

Line 6268: New --explain option

  • ✅ Boolean flag (no user input processed)

Line 6270-6274: Early return path

  • VERIFIED SAFE: Imports printGovernanceModel, calls it with no arguments, returns immediately
  • ✅ No I/O before return (no resource leaks)

Governance Model (src/soul/governance-model.ts)

Line 9: Import from ./scanner

  • ✅ Internal module (no dependency risk in this PR)

Line 33-56: buildDomainSummaries()

  • ✅ Pure function over static data structures
  • ✅ Throws on empty control set (defensive, not a vulnerability)

Line 67-98: renderGovernanceModel()

  • ✅ String formatting only, no eval/dynamic code generation
  • .join() calls are safe (array of strings)

Line 101-103: printGovernanceModel()

  • ✅ Writes to process.stdout (intended output stream)
  • ✅ No untrusted input in the output chain

PREVIOUS REVIEW

No previous review section provided.


ANALYSIS

This PR introduces a documentation feature (--explain) with comprehensive test coverage that validates command references across the codebase. The implementation is security-clean:

  1. No command injection vectors: All execFileSync calls use array-form arguments with hardcoded/validated inputs
  2. No path traversal: Paths are constructed from __dirname and resolved against known repo structure
  3. No ReDoS: All regex patterns are linear-time (verified via nested quantifier analysis)
  4. No resource leaks: The --explain path returns immediately after stdout write
  5. No prototype pollution: Only uses standard object literals and array operations
  6. Type safety: TypeScript interfaces properly constrain function signatures

The test file's mutation testing (mentioned in description) and explicit regression case (line 187) demonstrate strong quality gates.


VERDICT: APPROVE

SUMMARY: PR #166 adds a --explain flag to scan-soul that prints the 9-domain governance model, addressing a documentation gap where PR #142 cited a non-existent command. The implementation is read-only (no scan I/O), uses safe subprocess invocation patterns (array-form execFileSync, no shell), and includes a comprehensive cross-reference test that validates command/flag citations in concept-explainers. No security or correctness issues found after verification—all subprocess calls, path operations, and regex patterns are adequately controlled.


Reviewed 3 files changed (14143 bytes)

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

VERDICT: APPROVE

SUMMARY:
PR #166 adds the --explain flag to scan-soul that was previously cited in concept-explainer text but never implemented. The implementation is security-clean: subprocess invocations use safe array-form arguments with hardcoded inputs, all regex patterns are linear-time (no ReDoS risk), path operations are anchored to resolved repo root (no traversal), and the early-return execution path prevents resource leaks. The cross-reference test provides strong regression protection by validating that all command/flag citations in concept-explainers.ts resolve to registered Commander options.

PREVIOUS REVIEW STATUS:
The previous review flagged no genuine security issues—all findings were marked "VERIFIED SAFE" after checking mitigations. No changes were needed.


All verification checklists completed. No unmitigated CRITICAL or HIGH findings.


Reviewed 3 files changed (14143 bytes)

@thebenignhacker thebenignhacker merged commit bbbea43 into main Apr 29, 2026
1 check passed
@thebenignhacker thebenignhacker deleted the fix/scan-soul-explain-163 branch April 29, 2026 21:31
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.

concept-explainer cites scan-soul --explain (PR #142) but the option is not registered

1 participant