Skip to content

fix(report): restore bar-row hover cue safely - #828

Merged
seonghobae merged 14 commits into
mainfrom
fix/bar-row-hover-clean-813
Aug 17, 2026
Merged

fix(report): restore bar-row hover cue safely#828
seonghobae merged 14 commits into
mainfrom
fix/bar-row-hover-clean-813

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Clean reconstruction of the valid report-hover delta from closed unsafe PR #813, plus bounded report-rendering guards and a heading-ID XSS correction discovered while validating the branch.

Test-first lineage

  • RED 11958fdb22e52fc5be1cddd2046b416019fde4cb requires .bar-row padding, radius and hover background without opacity or peer dimming; GREEN 072712de3dce1adf4cd7f8c2f238f20c80dadc16 implements only that UX slice.
  • 89b25fadb8e427419283c30fed4fb8c4d99ea819 introduces row/column/cell/card bounds.
  • RED 1addf09b86eb8ec48ff203031ced671140899d67 proves sparse rows can exceed the materialized exact-value cell budget; GREEN 923e685dca7b99fcb86f22bf1d4c588f1d4963f5 validates rows × union(columns) before HTML/JSON/CSV construction.
  • c7aa7d28b8c01f40c91b6fdd4ea1af02ab61808f reconciles then-current protected main.
  • RED 1ac3f1616297a7f4b8d1be581433a9deba2be18b reproduces heading-ID attribute/script injection; GREEN / exact source head 4f37dd72cdce42c45801bd2272e3aaca12533d0c escapes generated metric and table heading IDs.

Scope and safety

Exact current state

  • Protected base: fb67ced09d8ee00542c05d56374537a9a7239751.
  • Exact source head: 4f37dd72cdce42c45801bd2272e3aaca12533d0c.
  • The branch is a direct descendant of protected main and the net diff is six report/test files.
  • GitHub reports the PR mergeable and Draft.
  • Target exact-head CI, security, static analysis, Python and emitted coverage/OpenCode check contexts are terminal with no failure or queue; no unresolved inline thread is present.
  • Formal OpenCode reviews on this exact head remain CHANGES_REQUESTED. Central runs 31833884627 and 31836935337 reported that required coverage evidence did not materialize. The separately successful exact-head check contexts do not supersede those formal review decisions. The blocker is central evidence/control-plane behavior rather than proof of a report-source defect, but it remains effective and is not dismissed.
  • Integration additionally requires every live required context and a qualifying independent non-author approval.

No gate, review or approval is weakened or bypassed. Any future source-head movement requires fresh exact-head evidence.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0aa07fe5-fc49-457a-8ae6-5c6da4532ce4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 74e5cfe3-ef09-4fb5-a406-43752d13498b

📥 Commits

Reviewing files that changed from the base of the PR and between 4f9276b and 072712d.

📒 Files selected for processing (2)
  • python/fast_mlsirm/report.py
  • tests/test_report_numeric_motion_css.py

📝 Walkthrough

Walkthrough

The report adds padding, rounded corners, and hover background styling to .bar-row. Regression tests verify the rendered CSS declarations.

Changes

Bar Row Styling

Layer / File(s) Summary
Bar row styling and regression coverage
python/fast_mlsirm/report.py, tests/test_report_numeric_motion_css.py
The report CSS updates .bar-row spacing, corner rounding, and hover background styling. Tests assert the new declarations.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Mergeability Score: ⚪ Minimal · up to 07271

This PR makes a narrowly scoped visual hover-cue change with a matching regression test; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the report fix and the restored bar-row hover cue, which matches the main change.
✨ 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 fix/bar-row-hover-clean-813

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.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor Author

Exact-head Strix rerun on 072712de3dce1adf4cd7f8c2f238f20c80dadc16 produced two real scanner findings, so this lane stays Draft; this is not a provider outage or a pass.

RCA / adjudication against live code:

  1. VULN-0001 path traversal — scanner threat model is not present in this repository. render_diagnostics_report(diagnostics_path, output_path) is a local library/CLI API whose caller intentionally selects the input and output paths; the repository contains no FastAPI/Flask/HTTP route exposing those arguments to an unauthenticated remote caller, and README explicitly positions fast-mlsirm as a local toolkit rather than a hosted assessment platform. Rejecting .. (the historical 🎨 Palette: 바 차트 행(Bar Row) 호버 시 시각적 피드백 추가 #813 side-change) would not establish a meaningful filesystem boundary and would break legitimate local relative-path use. Treat the scanner's invented unauthenticated POST surface as a false-positive threat model, not as justification for a cwd sandbox.

  2. VULN-0002 unbounded JSON DoS — the stated premise is already mitigated, but there is a narrower reliability question. render_diagnostics_report delegates to io._load_json_bounded; live io.py caps diagnostics JSON at 32 MiB and nesting depth 128 before json.loads. So the report does not accept unbounded JSON as the finding claims. However, exact_value_disclosure() deliberately materializes complete HTML + JSON + CSV exact-value views from the bounded source, so bounded-input output amplification remains a legitimate reliability-hardening candidate even though it is not the reported unauthenticated remote DoS.

Do not merge or mark Ready on the basis of this adjudication. The required Strix check remains red. The safe next remedy is to preserve the intended local path contract, separately bound exact-value rendering/output amplification with test-first resource limits if realistic acceptance data supports them, and then regenerate exact-head security evidence. The hover-only semantic change itself remains unrelated to either finding.

Copy link
Copy Markdown
Contributor Author

Fresh exact-head Strix RCA for 923e685dca7b99fcb86f22bf1d4c588f1d4963f5:

  • required strix run 31766071124 / job 94662214333 genuinely failed after the scanner completed; this is not a backend-unavailable placeholder;
  • the uploaded strix-reports artifact reports one MEDIUM CWE-22 finding against render_diagnostics_report(output_path=...) and proposes rejecting absolute paths / .. components;
  • the same report also states _load_json_bounded was called with an incorrect source= keyword, but protected-main io.py defines _load_json_bounded(..., *, source: str, ...), so that part of the scanner analysis is factually incorrect;
  • render_diagnostics_report explicitly documents both paths as caller-selected local filesystem paths and states that services accepting untrusted identifiers must map them to trusted filesystem paths before this boundary. There is no package-owned output-directory sandbox for output_path to escape, and existing direct API/CLI behavior intentionally supports caller-chosen absolute paths. Rejecting absolute paths or .. solely to satisfy this report would therefore be a breaking semantic change that does not establish a meaningful trusted root.

Classification: the reported CWE-22 path-boundary claim is not accepted as a source vulnerability on the current contract, but the required Strix check is still failure and is not treated as passing or overridden. Keep this PR Draft. A future same-head scanner result must either clear/supersede the false-positive claim or a separately designed secure service/output-root boundary must be added for an actual untrusted-path use case; do not weaken/bypass the required check.

Copy link
Copy Markdown
Contributor Author

Exact-head security evidence refresh for 923e685dca7b99fcb86f22bf1d4c588f1d4963f5:

  • Strix run 31766071124, attempt 2, completed successfully on this exact source head and uploaded its reports artifact.
  • The report records zero security vulnerabilities. Its separate reliability recommendation says a missing relative .io module causes an ImportError.
  • Fresh source inspection does not support that recommendation: the changed python/fast_mlsirm/report_exact_values.py imports the Python standard-library module with import io; it does not import .io. A fresh python/fast_mlsirm/scoring/essay tree inspection likewise finds no changed reference to a relative io module. Creating an io.py to satisfy that scanner narrative would invent a dependency not present in the exact diff.

RCA classification: genuine same-head Strix execution, zero reported vulnerabilities, but the .io reliability recommendation is an invalid interpretation of the exact source and requires no product mutation. Current exact-head check inventory has no queued/in-progress/failing check, and there are no formal reviews or unresolved inline threads at this observation. Independent non-author approval remains a separate protected-main merge gate.

@seonghobae
seonghobae marked this pull request as ready for review August 14, 2026 05:04

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head c7aa7d28b8c01f40c91b6fdd4ea1af02ab61808f.

  • Head SHA: c7aa7d28b8c01f40c91b6fdd4ea1af02ab61808f

  • Workflow run: 31779627977

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (2 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (2 files)"]
  R1 --> V1["required checks"]
  Evidence --> S2["Test (3 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (3 files)"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: 4f37dd72cdce42c45801bd2272e3aaca12533d0c
  • Workflow run: 31836935337
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 4f37dd72cdce42c45801bd2272e3aaca12533d0c.

  • Head SHA: 4f37dd72cdce42c45801bd2272e3aaca12533d0c

  • Workflow run: 31836935337

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (2 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (2 files)"]
  R1 --> V1["required checks"]
  Evidence --> S2["Test (4 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (4 files)"]
  R2 --> V2["targeted test run"]
Loading

@seonghobae
seonghobae marked this pull request as draft August 14, 2026 08:05
@seonghobae
seonghobae marked this pull request as ready for review August 14, 2026 08:05
@seonghobae
seonghobae enabled auto-merge (squash) August 14, 2026 08:05

Copy link
Copy Markdown
Contributor Author

@opencode-agent review the current exact PR head. Preserve all protected-main gates; do not merge or update the branch.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@seonghobae have exceeded the limit for the number of chat messages per hour. Please wait 5 minutes and 44 seconds before sending another message.

@opencode-agent
opencode-agent Bot disabled auto-merge August 14, 2026 08:56
@seonghobae
seonghobae enabled auto-merge (squash) August 14, 2026 09:03
@opencode-agent
opencode-agent Bot disabled auto-merge August 14, 2026 10:44
@seonghobae
seonghobae marked this pull request as draft August 14, 2026 13:00
@seonghobae
seonghobae marked this pull request as ready for review August 14, 2026 13:14
@seonghobae
seonghobae enabled auto-merge (squash) August 14, 2026 13:14

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 4f37dd72cdce42c45801bd2272e3aaca12533d0c.

  • Head SHA: 4f37dd72cdce42c45801bd2272e3aaca12533d0c

  • Workflow run: 31833884627

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (2 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (2 files)"]
  R1 --> V1["required checks"]
  Evidence --> S2["Test (4 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (4 files)"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 4f37dd72cdce42c45801bd2272e3aaca12533d0c.

  • Head SHA: 4f37dd72cdce42c45801bd2272e3aaca12533d0c

  • Workflow run: 31836935337

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file (2 files)"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file (2 files)"]
  R1 --> V1["required checks"]
  Evidence --> S2["Test (4 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (4 files)"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent
opencode-agent Bot disabled auto-merge August 14, 2026 21:06
@seonghobae
seonghobae marked this pull request as draft August 15, 2026 06:23
@seonghobae
seonghobae marked this pull request as ready for review August 15, 2026 15:04
@seonghobae
seonghobae marked this pull request as draft August 16, 2026 04:10

Copy link
Copy Markdown
Contributor Author

@opencode-agent Please re-review exact unchanged head 4f37dd72cdce42c45801bd2272e3aaca12533d0c against current protected central review implementation .github@c47afc2dc68488292c1db7c9d6f82dcd5360f181. The operative exact-head CHANGES_REQUESTED decisions came from runs 31833884627 and 31836935337 on older central review code and reported only failed/missing coverage evidence; the current source has zero unresolved inline thread. Keep the PR Draft until a fresh exact-head decision. Review only; do not update this branch or merge.

@seonghobae
seonghobae marked this pull request as ready for review August 16, 2026 20:06
@cursor

cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@seonghobae
seonghobae enabled auto-merge (squash) August 16, 2026 20:06
@seonghobae
seonghobae merged commit 07be285 into main Aug 17, 2026
35 checks passed
@seonghobae
seonghobae deleted the fix/bar-row-hover-clean-813 branch August 17, 2026 05:25
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.

1 participant