test(diagnostics): add hostile-input and browser assurance - #285
Draft
seonghobae wants to merge 45 commits into
Draft
test(diagnostics): add hostile-input and browser assurance#285seonghobae wants to merge 45 commits into
seonghobae wants to merge 45 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough릴리스 워크플로가 SPDX 2.3 SBOM 생성, 체크섬, 증명 및 자산 검증을 추가했습니다. Writing Diagnostics가 승인된 텍스트 범위를 선택합니다. 단위 테스트와 브라우저 테스트가 보안, 접근성 및 문서 변경 동작을 검증합니다. Changes릴리스 공급망 검증
Writing Diagnostics 동작 및 보증
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WritingDiagnosticsPanel
participant WritingDiagnosticsController
participant CwlEditor
User->>WritingDiagnosticsPanel: select Focus text
WritingDiagnosticsPanel->>WritingDiagnosticsController: focus verified diagnostic
WritingDiagnosticsController-->>WritingDiagnosticsPanel: approve or reject revision
WritingDiagnosticsPanel->>CwlEditor: select diagnostic range
WritingDiagnosticsPanel->>CwlEditor: focus editor and execute command
sequenceDiagram
participant ReleaseWorkflow
participant Cosign
participant Syft
participant GitHubRelease
ReleaseWorkflow->>Cosign: verify signing tool
ReleaseWorkflow->>Syft: generate SPDX 2.3 SBOM
Syft-->>ReleaseWorkflow: return inkspan.spdx.json
ReleaseWorkflow->>ReleaseWorkflow: create checksums and attestations
ReleaseWorkflow->>GitHubRelease: validate SBOM and release assets
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
.github/workflows/writing-diagnostics-assurance-tdd.yml (3)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win변경된 패널의 기존 테스트도 실행하세요.
이 단계는 신규 테스트 두 개만 실행합니다. 이번 PR은
src/components/WritingDiagnosticsPanel.tsx를 변경합니다. 해당 컴포넌트의 기존 스위트src/components/WritingDiagnosticsPanel.test.tsx는 실행되지 않습니다. 회귀를 잡기 위해 이 파일을 목록에 추가하세요.♻️ 제안 변경
pnpm exec vitest run src/components/writingDiagnosticsSecurity.test.tsx src/components/writingDiagnosticsFocus.test.tsx + src/components/WritingDiagnosticsPanel.test.tsx --pool=forks --maxWorkers=1🤖 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 @.github/workflows/writing-diagnostics-assurance-tdd.yml around lines 34 - 40, Update the “Run hostile-input, no-fallback, and focus assurance” Vitest command to include src/components/WritingDiagnosticsPanel.test.tsx alongside the two existing test files, while preserving the current fork pool and single-worker options.
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPR 트리거를 추가하세요.
트리거는 단일 브랜치
feat/writing-diagnostics-assurance의 push와 수동 실행뿐입니다. 브랜치를 병합하거나 이름을 변경하면 이 보증은 다시 실행되지 않습니다. 또한 PR을 게이트하지 않습니다. 관련 경로에 대한pull_request트리거를 추가하세요.♻️ 제안 변경
on: push: branches: - feat/writing-diagnostics-assurance + pull_request: + paths: + - 'src/components/**' + - 'tests/browser/**' + - '.github/workflows/writing-diagnostics-assurance-tdd.yml' workflow_dispatch:🤖 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 @.github/workflows/writing-diagnostics-assurance-tdd.yml around lines 3 - 7, Update the workflow triggers under on to add a pull_request trigger scoped to relevant path changes, while preserving the existing push trigger for feat/writing-diagnostics-assurance and workflow_dispatch. Ensure pull requests affecting the workflow’s relevant paths run the writing diagnostics assurance without changing the existing branch behavior.
48-63: 🧹 Nitpick | 🔵 Trivial브라우저 바이너리 캐시를 고려하세요.
PLAYWRIGHT_BROWSERS_PATH를/tmp아래로 지정했지만 캐시 단계가 없습니다. 따라서 모든 실행이 세 엔진을 새로 다운로드합니다. 잠금 파일 해시를 키로 하는actions/cache단계를 추가하면 실행 시간과 네트워크 사용을 줄일 수 있습니다. 캐시 적중 시에도 시스템 의존성 설치는 유지하세요.🤖 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 @.github/workflows/writing-diagnostics-assurance-tdd.yml around lines 48 - 63, 워크플로의 Playwright 설치 단계 전에 actions/cache 단계를 추가해 PLAYWRIGHT_BROWSERS_PATH 디렉터리를 캐시하고, tests/browser 잠금 파일 해시를 캐시 키에 포함하세요. 캐시 적중 여부와 관계없이 기존 playwright install --with-deps 명령을 유지해 시스템 의존성 설치가 계속 수행되도록 하세요.tests/browser/specs/writing-diagnostics.browser.spec.ts (1)
52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win프로젝트 이름 문자열에 의존하는 게이트를 강화하세요.
터치 타깃 검증은
testInfo.project.name.includes('mobile')조건에서만 실행됩니다.tests/browser/playwright.config.ts의 프로젝트 이름chromium-mobile-diagnostics를 변경하면 이 검증은 조용히 건너뛰어지고 테스트는 통과로 보고됩니다. 프로젝트 이름 대신use설정 값(예:testInfo.project.use.hasTouch또는 전용 메타데이터)으로 판정하거나, 두 파일이 공유하는 상수를 사용하세요.🤖 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/browser/specs/writing-diagnostics.browser.spec.ts` around lines 52 - 55, Replace the project-name check in the touch-target assertions with a stable capability or metadata-based condition, such as testInfo.project.use.hasTouch or a shared constant with the Playwright configuration. Ensure the width and height validations still run for the mobile diagnostics project even if its name changes.src/components/writingDiagnosticsFocus.test.tsx (1)
100-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
editor === null분기 테스트를 추가하세요.패널 가드는 단축 평가를 사용합니다.
focusDiagnostic이false를 반환하면editor === null조건은 평가되지 않습니다. 따라서 현재 두 테스트는 에디터가 없는 경로를 커버하지 않습니다.focusDiagnostic이true를 반환하고controller.editor가null인 컨트롤러로 한 개의 테스트를 추가하세요.♻️ 제안 테스트
+ it('does not throw when the controller has no editor', () => { + const controller = { + ...buildController(buildEditorProbe().editor, true), + editor: null, + } as WritingDiagnosticsController; + render( + <WritingDiagnosticsPanel + controller={controller} + label="Writing guidance" + />, + ); + + fireEvent.click( + screen.getByRole('button', { + name: 'Focus affected text for Clarify Alpha', + }), + ); + + expect(controller.focusDiagnostic).toHaveBeenCalledWith('focus-diagnostic'); + });이 의견은 코딩 가이드라인의 "Preserve exact owned production statement/branch/function/line coverage and public-docstring requirements enforced by repository CI" 규칙을 근거로 합니다.
🤖 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/components/writingDiagnosticsFocus.test.tsx` around lines 100 - 119, Extend the focus-diagnostic tests with one case where focusDiagnostic returns true but controller.editor is null, exercising the panel’s editor-null short-circuit branch. Reuse the existing render and button interaction setup, assert focusDiagnostic is called, and verify editor.chain and chain.run are not invoked.Source: Coding guidelines
tests/browser/harness.ts (1)
230-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win핸들이 없을 때 명시적 오류를 발생시키세요.
mountWritingDiagnosticsProbe는 사전 조건 실패 시writing_diagnostics_editor_unavailable같은 명시적 오류를 던집니다. 반면 이 세 헬퍼는 핸들이 없으면 조용히 무시하거나false/null을 반환합니다. 스펙은undo반환값을 확인하지 않습니다. 따라서 프로브가 마운트되지 않은 상태에서도 테스트가 다른 지점에서 모호하게 실패합니다. 마운트 함수와 같은 fail-closed 방식을 적용하세요.♻️ 제안 리팩터
+function requireDiagnosticsHandle(): CwlEditorHandle { + const handle = diagnosticsEditorRef.current; + if (handle === null) throw new Error('writing_diagnostics_probe_unmounted'); + return handle; +} + window.mutateInkspanWritingDiagnosticsProbe = (sourceHtml: string): void => { - diagnosticsEditorRef.current?.setValue(sourceHtml); + requireDiagnosticsHandle().setValue(sourceHtml); }; window.applyInkspanWritingDiagnosticProbe = ( diagnosticId: string, ): Promise<CwlWritingDiagnosticActionEvent | null> => - diagnosticsEditorRef.current?.applyWritingDiagnostic(diagnosticId) ?? - Promise.resolve(null); + requireDiagnosticsHandle().applyWritingDiagnostic(diagnosticId); -window.undoInkspanWritingDiagnosticsProbe = (): boolean => - diagnosticsEditorRef.current?.getEditor()?.commands.undo() ?? false; +window.undoInkspanWritingDiagnosticsProbe = (): boolean => { + const editor = requireDiagnosticsHandle().getEditor(); + if (editor === null) throw new Error('writing_diagnostics_editor_unavailable'); + return editor.commands.undo(); +};🤖 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/browser/harness.ts` around lines 230 - 241, Update mutateInkspanWritingDiagnosticsProbe, applyInkspanWritingDiagnosticProbe, and undoInkspanWritingDiagnosticsProbe to throw the same explicit writing_diagnostics_editor_unavailable error when diagnosticsEditorRef is unavailable, instead of silently no-oping or returning null/false. Preserve the existing operation results when the editor handle exists, without adding validation of the undo return value.
🤖 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 @.github/workflows/release.yml:
- Line 145: Update the release workflow’s SBOM generation around syft scan and
the release artifact publishing steps to scan each exact release/*.tgz and
release/*.whl individually instead of the entire workspace. Produce a separate
SBOM for each artifact, attach only its matching SBOM to that artifact, and
validate the package-to-SBOM relationships before publishing.
In `@src/components/writingDiagnosticsSecurity.test.tsx`:
- Around line 141-142: Update the assertions in the writing diagnostics security
test to query img[src="x"], matching the <img src=x onerror=alert(1)> payload
instead of script[src="x"]. Also assert that the explanation is rendered as
text, while preserving the existing HOST_CALLBACK_SECRET absence check.
In `@tests/browser/harness.ts`:
- Around line 2-16: Declare react and react-dom as direct dependencies of the
tests/browser package, updating its package manifest and pnpm lockfile so
harness.ts resolves both imports in isolated browser-test installations without
relying on the root workspace.
---
Nitpick comments:
In @.github/workflows/writing-diagnostics-assurance-tdd.yml:
- Around line 34-40: Update the “Run hostile-input, no-fallback, and focus
assurance” Vitest command to include
src/components/WritingDiagnosticsPanel.test.tsx alongside the two existing test
files, while preserving the current fork pool and single-worker options.
- Around line 3-7: Update the workflow triggers under on to add a pull_request
trigger scoped to relevant path changes, while preserving the existing push
trigger for feat/writing-diagnostics-assurance and workflow_dispatch. Ensure
pull requests affecting the workflow’s relevant paths run the writing
diagnostics assurance without changing the existing branch behavior.
- Around line 48-63: 워크플로의 Playwright 설치 단계 전에 actions/cache 단계를 추가해
PLAYWRIGHT_BROWSERS_PATH 디렉터리를 캐시하고, tests/browser 잠금 파일 해시를 캐시 키에 포함하세요. 캐시 적중
여부와 관계없이 기존 playwright install --with-deps 명령을 유지해 시스템 의존성 설치가 계속 수행되도록 하세요.
In `@src/components/writingDiagnosticsFocus.test.tsx`:
- Around line 100-119: Extend the focus-diagnostic tests with one case where
focusDiagnostic returns true but controller.editor is null, exercising the
panel’s editor-null short-circuit branch. Reuse the existing render and button
interaction setup, assert focusDiagnostic is called, and verify editor.chain and
chain.run are not invoked.
In `@tests/browser/harness.ts`:
- Around line 230-241: Update mutateInkspanWritingDiagnosticsProbe,
applyInkspanWritingDiagnosticProbe, and undoInkspanWritingDiagnosticsProbe to
throw the same explicit writing_diagnostics_editor_unavailable error when
diagnosticsEditorRef is unavailable, instead of silently no-oping or returning
null/false. Preserve the existing operation results when the editor handle
exists, without adding validation of the undo return value.
In `@tests/browser/specs/writing-diagnostics.browser.spec.ts`:
- Around line 52-55: Replace the project-name check in the touch-target
assertions with a stable capability or metadata-based condition, such as
testInfo.project.use.hasTouch or a shared constant with the Playwright
configuration. Ensure the width and height validations still run for the mobile
diagnostics project even if its name changes.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71c5a3e3-0ea3-4cac-9da4-783f6ede9486
📒 Files selected for processing (8)
.github/workflows/release.yml.github/workflows/writing-diagnostics-assurance-tdd.ymlsrc/components/WritingDiagnosticsPanel.tsxsrc/components/writingDiagnosticsFocus.test.tsxsrc/components/writingDiagnosticsSecurity.test.tsxtests/browser/harness.tstests/browser/playwright.config.tstests/browser/specs/writing-diagnostics.browser.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack dependency
This Draft PR is stacked on #282 (
feat/writing-diagnostics-package) and implements Task 9 of the accepted writing-diagnostics plan. It verifies the host-supplied diagnostic boundary; it does not give Inkspan semantic authority or introduce a model/provider path.Included
Security and authority boundary
Review findings addressed
All actionable CodeRabbit threads and follow-up review findings are resolved:
Release-workflow RCA
The inherited stack contained the predecessor release workflow that GitHub rejected before creating any job. The branch carries the canonical executable release workflow and then incorporates the reviewed SBOM/provenance hardening without weakening tag-only release gates.
Exact-head verification
Current immutable contributor head:
Direct branch-push run
31649800097completed successfully on that exact head:6e74a9ba9381da48ddafdabcbe203343f6afa304with persisted credentials disabled;tests/browser/pnpm-lock.yamldigest;PR run
31649803245separately validated the synthetic merge result699260873a5dc64cab4882308b04514890ea3606against current basecb49b1a6d646b5ba15f6aa88e568adf323fe05fc; it is merge-result compatibility evidence, not a substitute for the immutable contributor-head run above.Merge boundary
Keep this PR Draft while #282 and its stacked dependencies remain Draft and release issue #118 freezes publication. Independent review, branch protection, every exact-current-head required check, and the immutable release sequence remain mandatory. This PR does not publish npm, create a tag or GitHub Release, or change package version.
Summary by CodeRabbit
개선 사항
버그 수정
품질 개선