fix(import): accept XML delimiter whitespace - #467
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughMS Project XML 파서가 태그명 뒤의 XML 공백을 허용하고 정확한 ChangesMS Project XML 파싱
Hono 의존성 갱신
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MSProjectXML
participant XMLTagScanner
participant MSProjectParser
participant ParsedTasks
MSProjectXML->>XMLTagScanner: 태그명과 XML 공백 뒤의 > 탐색
XMLTagScanner->>MSProjectParser: 정확한 시작 태그와 종료 태그 전달
MSProjectParser->>ParsedTasks: Task 및 하위 블록 파싱
ParsedTasks-->>MSProjectParser: 작업 ID, 단계명, 날짜, 선행 작업 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cloud-sync.js`:
- Around line 775-780: Update the block extraction logic around findTagBoundary
so a closing tag is accepted only when no subsequent same-name opening tag
appears before it; otherwise stop at the unmatched outer opening and do not
collect that block. Add a regression test covering an incomplete outer Task
followed by a complete nested Task block.
In `@tests/unit/msproject.test.mjs`:
- Around line 99-103: Update the malformed XML fixture in the parseMsProjectXml
test to contain an actual U+000B vertical tab using the JavaScript \u000B escape
instead of the literal backslash-v sequence, while preserving the expected empty
result and assertion message.
🪄 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: 22909d81-5171-403f-b335-1b5d6400849d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
CHANGELOG.mdcloud-sync.jsdocs/security.mdpackage.jsontests/unit/msproject.test.mjs
|
@coderabbitai review Please review exact current head |
|
@opencode-agent review Please submit an independent formal review for exact current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base='24c86489800ef4405e737a8e3d5bc3d54d7628dd'
head='24512574696bda29845053bc39aa0530c1c7f532'
echo '=== Effective changed files ==='
git diff --name-status "$base" "$head"
echo '=== Effective diff ==='
git diff --find-renames --find-copies --unified=80 "$base" "$head" -- \
cloud-sync.js tests/unit/msproject.test.mjs docs/security.md CHANGELOG.md package.json
echo '=== Parser structure ==='
ast-grep outline cloud-sync.js --items all --type function --match 'parseMsProjectXml|tag|collectBlocks|predecessorIds' || true
echo '=== Parser references ==='
rg -n -C 5 -P '\b(parseMsProjectXml|collectBlocks|predecessorIds|function\s+tag|const\s+tag)\b' cloud-sync.js
echo '=== Test structure ==='
ast-grep outline tests/unit/msproject.test.mjs --items all --type function || trueLength of output: 26905 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='24512574696bda29845053bc39aa0530c1c7f532'
echo '=== GitHub check runs for exact head ==='
gh api "repos/ContextualWisdomLab/scopeweave/commits/${head}/check-runs?per_page=100" \
--jq '.check_runs[] | [.name, .status, .conclusion, .html_url] | `@tsv`' || true
echo '=== GitHub combined status for exact head ==='
gh api "repos/ContextualWisdomLab/scopeweave/commits/${head}/status" \
--jq '.state, (.statuses[]? | [.context, .state, .target_url] | `@tsv`)' || trueLength of output: 5170 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='24512574696bda29845053bc39aa0530c1c7f532'
gh api "repos/ContextualWisdomLab/scopeweave/commits/${head}/check-runs?per_page=100" \
--jq '.check_runs[] | select(.name == "strix") | [.name, .status, (.conclusion // "none"), .html_url] | `@tsv`'Length of output: 531
I found no code findings. The effective diff changes only the XML scanner, regression tests, security guidance, and The scanner enforces exact tag-name matching. It accepts only space, tab, CR, and LF before Exact-head workflow verification is incomplete. The
|
2451257 to
ededaee
Compare
|
@opencode-agent @cwl-noema-review Review exact clean head |
|
@opencode-agent review Please submit an independent formal review for exact current head |
Pull request was converted to draft
|
@opencode-agent |
|
@opencode-agent Review exact current head |
|
@opencode-agent @cwl-noema-review Please perform a fresh independent formal review of exact current head |
OpenCode Review Overview
--> Changed-File Evidence Mapflowchart 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["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test: msproject.test.mjs"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: msproject.test.mjs"]
R3 --> V3["targeted test run"]
|
Pull request was converted to draft
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head Server Tests run 31869862197 now provides the intended RED evidence for the regression-only head 3491f51959a97a4f7bfcc32394b07b130ed6c395: tests/unit/msproject.test.mjs:116 fails because the scalar parser accepts <Name>outer<Name>inner</Name> as outer<Name>inner. The production fix must mirror collectBlocks()' same-name nesting invariant in tag(); do not weaken or remove the regression. After the source fix, rerun the full exact-head repository/security/coverage gates before returning this Draft to Ready.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
APPROVE PR #467 (fix(import): accept XML delimiter whitespace). Changed-file evidence inspected: cloud-sync.js (isXmlWhitespace/findTagBoundary scanner around parseMsProjectXml at cloud-sync.js:741), tests/unit/msproject.test.mjs (whitespace, lookalike-name, U+000B, truncation, and nested same-name cases), CHANGELOG.md:56-59, docs/security.md:22, docs/doctoring/ms-project-xml-import-boundary.md. The scanner accepts only U+0020/U+0009/U+000D/U+000A between an exact element name and '>', rejects attributes, longer names, non-XML whitespace, nested unmatched same-name blocks, and truncated input, and stays monotone and regex-free. CodeGraph: parseMsProjectXml (cloud-sync.js:741) has 2 callers in cloud-sync.js and is covered by tests/unit/msproject.test.mjs; importMsProjectFile (cloud-sync.js:832) is unchanged. Approval sufficiency: yes — code, tests, security doc, decision record, and CHANGELOG all match the PR intent and each other. Verification posture: trusted Coverage execution evidence Result: PASS (supported repository test suites passed on head 5c9a3f1); no completed failed GitHub Checks; mergeStateStatus blocked is branch policy/review state, not a conflict (mergeable: true). Linter/static: no lint findings; only hadolint Dockerfile is configured and this PR touches no Docker files. TDD/regression: nested-scalar rejection (tests/unit/msproject.test.mjs:116-121) was driven by a prior RED run on regression head 3491f51 (run 31869862197) and mirrors the collectBlocks guard; whitespace cases cover space/tab/CR/LF on Task, UID, Name, OutlineLevel, Start, Finish, PredecessorLink, and PredecessorUID tags. Coverage: Coverage execution evidence Result: PASS with supported repository test suites passed. Docstring coverage: configured repository docstring gates passed or advisory per Coverage execution evidence. DAG: flowchart LR; A['MS Project XML upload'] --> B['parseMsProjectXml (cloud-sync.js:741)']; B --> C['findTagBoundary: XML-whitespace + exact 0x3e (cloud-sync.js:744)']; C --> D['tag/collectBlocks nested & truncation guards']; C -->|rejected| F['[] no tasks (linear, DoS-safe)']; D --> E['ScopeWeave task rows (msp-UID)']; D -->|rejected| F; E --> V['node tests/unit/msproject.test.mjs (Coverage PASS)'] — reflects base-to-head changed flow (byte-exact delimiter scanning broadened to XML whitespace; nested same-name handling tightened). PoC/execution: unit suite passed on this exact head; no package or lockfile change. DDD/domain: import-adapter boundary recorded in docs/doctoring/ms-project-xml-import-boundary.md with rollback instructions. CDD/context: docs/security.md XML-import section updated consistently with the scanner behavior. Similar issues: prior CodeRabbit parser findings (nested scalar/block pairing) are resolved on this head — current-head thread evidence lists no unresolved threads. Claim/concept check: accepted code points equal the XML 1.0 S production (#x20|#x9|#xD|#xA) and match the W3C Fifth Edition reference cited in the decision record. Standards search: XML 1.0 (Fifth Edition) reference present in the decision record. Compatibility/convention: no DB/API/config objects added; new locals (isXmlWhitespace, findTagBoundary, opening/closing/nextOpening, delimiter, searchFrom) are idiomatic camelCase and not reserved words; byte-exact exports keep identical ids, names, dates, parents, progress, and predecessors. Breaking-change/backcompat: only malformed-input behavior tightens (nested same-name elements now yield no value), documented in CHANGELOG and the decision record. Performance: scanner remains linear — constant bounded indexOf/slice passes per block with monotone searchFrom advance; the 5000-unclosed-block case at tests/unit/msproject.test.mjs:71-73 asserts no hang. Developer experience: test-file header documents the run command (node tests/unit/msproject.test.mjs); decision record documents the boundary and rollback. User experience: standards-compliant MS Project exports with formatting whitespace before '>' now import instead of silently yielding zero tasks. Visual/DOM: non-web change — the reviewed interaction surface is the import-parser CLI/API path and its node test harness; no DOM/visual impact. Accessibility/i18n: no UI change; Korean task-name fixtures in the tests remain intact. Supply-chain/license: no dependency changes. Packaging: no package or lockfile change. Security/privacy: no new identifier exposure or auth surface; import path remains regex-free and linear, preserving the ReDoS denial-of-service boundary documented in docs/security.md; U+000B and attribute input are rejected at the delimiter.
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including CHANGELOG.md, cloud-sync.js, docs/doctoring/ms-project-xml-import-boundary.md, docs/security.md, tests/unit/msproject.test.mjs.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects CHANGELOG.md to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"cloud-sync.js","line":744,"hypothesis":"Crafted input with thousands of repeated unclosed <Task> blocks causes quadratic rescanning or a hang in the broadened scanner.","attack_or_counterexample":"'<Project><Tasks>' + '<Task><UID>9</UID><Name>open</Name>'.repeat(5000) + '</Tasks></Project>' (the exact trusted regression input at tests/unit/msproject.test.mjs:71-73).","evidence":"Trusted unit suite passed under Coverage execution evidence (Result: PASS, supported repository test suites passed on head 5c9a3f1c); tests/unit/msproject.test.mjs:71-73 executed the 5000-unclosed-block input and asserted deepEqual(parseMsProjectXml(input), []) with the scanner completing (no hang), and the source trace shows findTagBoundary around cloud-sync.js:744 advances searchFrom monotonically past every inspected byte so the missing closing tag yields a single linear indexOf pass rather than a rescan; source-line-sha256=38b4147ab3278cf9384b3a7fb8079b59a5d927ca62efba25cfecadd5fec3ee5a","outcome":"falsified"},{"path":"tests/unit/msproject.test.mjs","line":75,"hypothesis":"An unmatched outer <Task> block pairs with a nested <Task>'s closing tag and silently produces a mis-parented task from malformed input.","attack_or_counterexample":"'<Project><Tasks><Task><UID>11</UID><Name>unclosed outer</Name><Task><UID>12</UID><Name>nested</Name></Task></Tasks></Project>'.","evidence":"Trusted unit suite passed (Coverage execution evidence Result: PASS); tests/unit/msproject.test.mjs:75-81 asserts parseMsProjectXml of that input deep-equals [] ('an unmatched outer Task cannot consume a nested Task closing tag'), matching the collectBlocks guard that stops at the first unmatched same-name opening instead of pairing it with a later block's closing tag; source-line-sha256=cc344544257d020b5c61ba1982850cda041d79a1a81588f3001c1fae1767b6ba","outcome":"falsified"},{"path":"cloud-sync.js","line":744,"hypothesis":"The broadened delimiter scan accepts non-XML whitespace (U+000B) or attribute-style text before '>', violating the documented import boundary and broadening the attack surface.","attack_or_counterexample":"'<Project><Tasks><Task\\u000B><UID>9</UID><Name>wrong whitespace</Name></Task\\u000B></Tasks></Project>' and '<Project><Tasks><TaskX><UID>9</UID><Name>wrong</Name></TaskX></Tasks></Project>'.","evidence":"Trusted unit suite passed (Coverage execution evidence Result: PASS); tests/unit/msproject.test.mjs:106-110 asserts the U+000B-delimited input deep-equals [] and tests/unit/msproject.test.mjs:101-105 asserts the <TaskX> lookalike yields []; the source trace at cloud-sync.js:744-746 (isXmlWhitespace predicate) accepts only 0x20/0x09/0x0d/0x0a and the follow-on boundary check requires exactly 0x3e next, so U+000B and attribute characters fall through to monotone rejection; source-line-sha256=38b4147ab3278cf9384b3a7fb8079b59a5d927ca62efba25cfecadd5fec3ee5a","outcome":"falsified"}],"residual_risk":"Bounded: importMsProjectFile (cloud-sync.js:832) has no dedicated unit test (pre-existing, unchanged by this PR); attribute-form rejection (<Task a=\"b\">) is exercised only through the scanner mechanism, not a dedicated test case; String.trim() still strips non-XML whitespace inside element content exactly as before this PR (content trimming was not part of the delimiter boundary change)."}- Result: APPROVE
- Reason: MS Project XML delimiter change is correct, linear, well-tested, and consistently documented across code, security.md, decision record, and CHANGELOG; trusted unit suite passed on this head and no failed checks or unresolved threads exist.
- Head SHA:
5c9a3f1c6703c82fd1e61c52bbc153a88586fa88 - Workflow run: 31933244848
- Workflow attempt: 1
1 participant
Buyer impact
Standards-compliant Microsoft Project XML may contain space, tab, carriage return, or line feed between an exact element name and
>. ScopeWeave's byte-exact scanner previously reported such exports as containing no tasks.This PR broadens only that delimiter boundary while preserving the linear, regex-free import profile and its malformed-input protections.
Closes #410 only after protected integration.
Exact current state
Protected base:
develop@b88e66e81e9701404d29a0f5de4f58573ceee14f.Current contributor head:
766eb2d2d48d3eaef66fdb093b5aa54abf3ce55c.The current protected-base PR diff remains limited to five intended XML-import files:
cloud-sync.jstests/unit/msproject.test.mjsdocs/security.mdCHANGELOG.mddocs/doctoring/ms-project-xml-import-boundary.mdNo Hono, package, lockfile, workflow, server, authentication, Clearfolio, attachment, or other security-runtime change is part of the current PR diff. A short-lived attempt to add an attachment-view authentication regression after a central security finding was deliberately removed rather than broadening this parser PR or competing with the dedicated #413 access-grant architecture; package/test registration was restored. The resulting PR diff is still the same five parser/documentation files.
Parser contract
Task,PredecessorLink, and scalar names remain required;>;TDD and parser repair evidence
Regression-only head
3491f51959a97a4f7bfcc32394b07b130ed6c395established the missing scalar invariant. Exact-head Server Tests run31869862197observed RED attests/unit/msproject.test.mjs:116:<Name>outer<Name>inner</Name>was incorrectly accepted asouter<Name>inner.The parser repair mirrors the already-hardened block invariant:
tag()resolves both the candidate same-name closing delimiter and the next same-name opening delimiter and returns an empty scalar when the next opening precedes the close. The regression remains alongside XML-whitespace, exact-name, U+000B, truncation, block-nesting, scalar, and predecessor cases.All CodeRabbit inline findings on the parser diff are resolved; the three review threads are resolved/outdated as appropriate. There is no qualifying independent current-head approval.
Security-gate finding discovered after parser GREEN
Predecessor parser head
e8182369c5078587b726322ee1a8a3e8e910f29dhad terminal-success repository-native parser/test/coverage checks, but the central exact-head Strix gate failed. Its artifact identified a valid repository-wide HIGH finding: the attachment viewer still places the general session JWT in a URL query parameter for a new-tab navigation path. That broad bearer credential can leak through browser history, logs, telemetry, intermediaries, screenshots, or copied URLs.This finding is not introduced by this five-file XML diff; it exists in protected behavior and therefore remains a real repository-owned security debt rather than something that may be ignored to merge this PR. The causal remediation belongs to the purpose-bound credential work tracked by #413. PR #506 currently provides the short-lived opaque access-grant domain groundwork, including purpose/audience/resource binding, one-time consumption, expiry, membership-version checks, and hash-only persistence. The runtime migration of attachment-view URL credentials still needs to land on that security architecture; stuffing an independent auth redesign into this XML parser branch would create competing authority and an unrelated semantic blast radius.
The Strix report's prose must not be treated as proof of remediation: its execution log shows attempted client patches failed to apply, so the protected attachment-view JWT-in-query behavior remains unfixed.
Current exact-head verification state
The current head
766eb2d2...was created after restoring the XML-only PR scope, so predecessor success does not transfer. At the last exact-current-head refetch, all six repository-native workflows were pending or queued: Server Tests, Fuzz, OSV Scanner, Dependency Review, Security Scan, and SAST Semgrep. The broader check suite was also still being created/executed. Pending, queued, skipped-required, neutral, failed, absent, stale, or predecessor evidence is non-passing.The GitHub UI currently shows this PR as Ready for review, but that UI state is not a merge-readiness claim. The unresolved repository-wide Strix security finding, current-head in-flight evidence, and missing independent last-push approval are hard merge blockers under the live rulesets.
Standards, compatibility, and rollback
docs/doctoring/ms-project-xml-import-boundary.mdrecords the supported XML profile, complexity/security boundary, compatibility, rollback, and the XML 1.0 Fifth Edition reference in APA 7th form.Rollback reverts only the scanner, focused regressions, security guidance, doctoring record, and CHANGELOG entry. It does not touch persisted project data or server contracts.
Merge gate
Do not integrate until the unchanged current head satisfies every applicable live required check and security gate, including resolution of the valid attachment-view credential finding through the dedicated access-grant/runtime path, zero valid unresolved review findings, and a qualifying current-head independent approval from someone other than the last pusher under the live protected-branch rules. Pending, skipped-required, neutral, failed, stale, predecessor-head, author-only, status-only, synthetic, or model-only evidence is non-passing.