Skip to content

Make control comments editable - #8278

Open
ejaronne wants to merge 7 commits into
masterfrom
codex/editable-comments
Open

Make control comments editable#8278
ejaronne wants to merge 7 commits into
masterfrom
codex/editable-comments

Conversation

@ejaronne

@ejaronne ejaronne commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR makes the control Comments field editable in the Heimdall frontend. The field is now always shown under the Details tab, even when empty, and user edits are reflected in exported/saved results.

Features

  • Adds an editable Comments textarea to control details.
  • Keeps comments visible even when the source value is null/empty.
  • Writes edited comments back into OHDF control descriptions.
  • Preserves edited comments in JSON export.
  • Updates CKL passthrough comments so DISA Checklist export includes edits.
  • Tracks unsaved comment edits per loaded file.
  • Shows an unsaved-edit indicator in the loaded file sidebar.
  • Warns users before removing dirty loaded results or closing the browser.
  • Enables saving DB-backed files after comment edits.
    -- NOTE: changes adds a server-mode reviewed-copy workflow: DB-backed comment edits create a new evaluation with filename <original filename> - review <YYYY-MM-DD HH-mm> and tags heimdall:review, heimdall:review-root:<id>, and heimdall:review-parent:<id>, instead of overwriting the original evaluation.
  • Clears dirty state after successful save/export.
  • Adds warning coverage for deleting a DB-backed loaded file with unsaved edits.

Approach

The implementation stores comment edits through the existing Vuex-backed loaded file state. Comment updates mutate the control’s OHDF description data, update the display wrapper, and, when the file came from CKL, update the CKL passthrough comment data used by CKL export.

Dirty-state tracking was added at the loaded-file level using hasUnsavedChanges, with supporting store getters/actions for save, export, remove, delete, and browser-unload flows.

Reviews Performed

Three focused reviews were performed:

  • Correctness/regression review
  • DRY, maintainability, and frontend best-practices review
  • Security, privacy, and data-integrity review

Findings addressed from review:

  • Added a prompt before deleting a DB-loaded file with unsaved comment edits.
  • Added error handling for failed saves of existing DB-backed files.
  • Tightened CKL passthrough matching to reduce risk of updating the wrong vulnerability.
  • Confirmed no obvious XSS path from the editable comments field; comments are rendered through Vue text rendering or existing sanitization paths.

Notes

The remaining broader hardening item is CKL structured-comment semantics: user text containing reserved section markers such as CAVEAT :: can still be interpreted by existing CKL parsing logic on a future round trip. That is pre-existing format behavior and may be better handled as a converter-level follow-up.

@ejaronne
ejaronne force-pushed the codex/editable-comments branch 2 times, most recently from 3ee839d to 04a0651 Compare June 16, 2026 04:09
@ejaronne
ejaronne force-pushed the codex/editable-comments branch from 04a0651 to e7f6ab3 Compare June 16, 2026 04:43
@ejaronne
ejaronne marked this pull request as ready for review June 16, 2026 05:10
@Amndeep7
Amndeep7 temporarily deployed to heimdall2-codex-editabl-0cz9h0 June 16, 2026 17:22 Inactive
@aaronlippold

Copy link
Copy Markdown
Member

I am reviewing this. I will let you know when I am done.

@ejaronne

Copy link
Copy Markdown
Contributor Author

I am reviewing this. I will let you know when I am done.

I'm about to commit some changes for the server workflow

@wdower
wdower temporarily deployed to heimdall2-codex-editabl-0cz9h0 June 16, 2026 23:18 Inactive
@ejaronne

ejaronne commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

I challenged Codex about my concerns to prove it was considering the current architecture and reusing existing architectural modules. It's response:

That concern is fair, and honestly healthy. AI-generated code can absolutely drift into “works in the demo, awkward in the codebase” territory if it is not held to normal maintainer standards.

For this PR, the strongest reassurance is that the implementation mostly does reuse Heimdall’s existing architecture rather than inventing a parallel path:

  • Comments update the existing in-memory OHDF/control model.
  • CKL preservation uses the existing passthrough data already produced by the mapper.
  • Unsaved state is tracked in the existing Vuex data store.
  • Save behavior reuses the existing /evaluations create endpoint and existing multipart form pattern.
  • Review lineage uses existing evaluation tags rather than adding schema.
  • Browser-close protection uses the standard beforeunload mechanism.
  • Remove/delete prompts are attached to existing sidebar/database-delete flows.

The right way to frame it to your colleague is not “Codex wrote it, trust it.” It’s:

  • We used Codex to draft an implementation, then constrained it to existing Heimdall frontend/server patterns. The PR still needs normal maintainer review for architecture fit, edge cases, and tests.

I’d also be candid about the risk areas:

  • The reviewed-copy workflow is new product behavior and should be reviewed by maintainers.
  • The tag convention is lightweight traceability, not true audit lineage.
  • The CKL passthrough matching logic deserves careful correctness review.
  • The UI save behavior should be checked against existing server-mode expectations.
  • E2E coverage may need to be expanded if this workflow becomes accepted.

That posture usually lands better: humble, specific, and reviewable. Not “the robot says it’s fine,” but “here are the architectural hooks it uses, and here are the exact places I’d like your eyes.”

aaronlippold added a commit that referenced this pull request Jun 17, 2026
…f-converters

Add setControlDescription (write-side complement to getDescription),
sanitizeCklSectionMarkers (prevents CKL section injection on round-trip),
and syncChecklistVulnComments (export-time passthrough sync using existing
ChecklistVuln types). 21 tests covering array-form, object-form, structured
comment preservation, marker escaping, and multi-field edits.

Part of epic heimdall2-9go: GUI Attestation & Description Editing Engine.
Supersedes the ad-hoc CKL parsing in PR #8278.

Authored by: Aaron Lippold<lippold@gmail.com>
aaronlippold added a commit that referenced this pull request Jun 17, 2026
Comprehensive Architecture Decision Record covering the attestation and
comment engine that replaces PR #8278. Three rounds of multi-agent review
(8 independent reviewers) with all findings resolved.

Covers: business workflows (attest NR controls, comment any control),
review lifecycle (Draft→InReview→Final), UX design (right-side review
panel, control row actions, notification bar), data architecture
(annotation store with O(1) indexed lookups, immutable original + overlay),
export/import (SAF CLI compatible attestation files + Heimdall annotation
bundles), CKL round-trip support, Phase 3 approval flow design.

Documents existing code inventory, prior art (checklistView branch),
and complete Phase 1-3 card breakdown with dependencies.

Authored by: Aaron Lippold<lippold@gmail.com>
@wdower
wdower temporarily deployed to heimdall2-codex-editabl-0cz9h0 June 17, 2026 04:16 Inactive
@wdower
wdower temporarily deployed to heimdall2-codex-editabl-0cz9h0 June 17, 2026 04:46 Inactive
@wdower
wdower temporarily deployed to heimdall2-codex-editabl-0cz9h0 June 17, 2026 05:03 Inactive
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
2 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@wdower
wdower temporarily deployed to heimdall2-codex-editabl-wb2d9j June 22, 2026 17:10 Inactive
aaronlippold added a commit that referenced this pull request Jun 23, 2026
… UI → integration

Each layer fully testable before the next starts. Opposite of PR #8278
(UI first) and checklistView (everything at once).

Authored by: Aaron Lippold<lippold@gmail.com>
aaronlippold added a commit that referenced this pull request Jun 23, 2026
…f-converters

Add setControlDescription (write-side complement to getDescription),
sanitizeCklSectionMarkers (prevents CKL section injection on round-trip),
and syncChecklistVulnComments (export-time passthrough sync using existing
ChecklistVuln types). 21 tests covering array-form, object-form, structured
comment preservation, marker escaping, and multi-field edits.

Part of epic heimdall2-9go: GUI Attestation & Description Editing Engine.
Supersedes the ad-hoc CKL parsing in PR #8278.

Authored by: Aaron Lippold<lippold@gmail.com>
aaronlippold added a commit that referenced this pull request Jun 23, 2026
Comprehensive Architecture Decision Record covering the attestation and
comment engine that replaces PR #8278. Three rounds of multi-agent review
(8 independent reviewers) with all findings resolved.

Covers: business workflows (attest NR controls, comment any control),
review lifecycle (Draft→InReview→Final), UX design (right-side review
panel, control row actions, notification bar), data architecture
(annotation store with O(1) indexed lookups, immutable original + overlay),
export/import (SAF CLI compatible attestation files + Heimdall annotation
bundles), CKL round-trip support, Phase 3 approval flow design.

Documents existing code inventory, prior art (checklistView branch),
and complete Phase 1-3 card breakdown with dependencies.

Authored by: Aaron Lippold<lippold@gmail.com>
aaronlippold added a commit that referenced this pull request Jun 23, 2026
… UI → integration

Each layer fully testable before the next starts. Opposite of PR #8278
(UI first) and checklistView (everything at once).

Authored by: Aaron Lippold<lippold@gmail.com>
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.

4 participants