Skip to content

fix(graph-vfs): decode graph VFS paths safely so a malformed % never throws - #323

Open
Diwak4r wants to merge 5 commits into
legioncodeinc:mainfrom
Diwak4r:fix/graph-vfs-malformed-percent-decode
Open

fix(graph-vfs): decode graph VFS paths safely so a malformed % never throws#323
Diwak4r wants to merge 5 commits into
legioncodeinc:mainfrom
Diwak4r:fix/graph-vfs-malformed-percent-decode

Conversation

@Diwak4r

@Diwak4r Diwak4r commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Problem

handleGraphVfs (src/daemon/runtime/codebase/query.ts) documents that an unknown/odd path "returns a short usage listing (never throws)", and its VFS caller resolveGraph (src/daemon-client/vfs/read.ts) wraps it in no try/catch — its own docblock repeats "The renderer itself never throws".

But parsePath runs the path remainder through decodeURIComponent, which raises URIError: URI malformed on a lone or short % escape. So a perfectly ordinary caller-supplied VFS path like graph/find/100% (or graph/show/a%zz, graph/neighborhood/src/x%.ts) throws out of handleGraphVfs instead of rendering — breaking the documented contract and propagating out of an un-guarded caller.

> decodeURIComponent('100%')
Uncaught URIError: URI malformed

Fix

Route the remainder through a small safeDecode helper that returns the raw string when decodeURIComponent throws, so an undecodable remainder is passed through verbatim rather than blowing up the whole render. One-line call-site change plus the helper.

How verified

  • Added a regression test alongside the existing "never throws" case in tests/daemon/runtime/codebase/query.test.ts covering find/100%, show/a%zz, and neighborhood/src/x%.ts.
  • npx vitest run tests/daemon/runtime/codebase/query.test.ts → 26 passed.
  • Confirmed no new typecheck/lint issues attributable to this change (the pre-existing useIterableCallbackReturn warning in this file is unrelated and present on main).

Summary by CodeRabbit

  • Bug Fixes

    • Prevented graph paths containing malformed percent-encoded sequences from causing errors.
    • Preserved the original path text when decoding is not possible, allowing requests to complete successfully.
  • Tests

    • Added coverage for malformed escape sequences in graph paths.
    • Improved test reliability for log-store retention, restart scenarios, health checks, and longer-running service operations by allowing additional execution time where needed.

…throws

handleGraphVfs documents that it "never throws" and its VFS caller
(resolveGraph) has no try/catch, but parsePath ran the remainder through
decodeURIComponent, which raises URIError on a lone/short % escape (e.g.
a `find/100%` pattern). Wrap the decode in a safeDecode helper that falls
back to the raw string, honoring the never-throws contract.
Copilot AI review requested due to automatic review settings July 26, 2026 11:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e25d4668-7a18-42e4-955a-807ab8d5c4c9

📥 Commits

Reviewing files that changed from the base of the PR and between 8e36179 and 563c236.

📒 Files selected for processing (1)
  • tests/cli/health-probes.test.ts

📝 Walkthrough

Walkthrough

parsePath() now preserves malformed percent-encoded graph path remainders. Tests cover invalid escapes. Log-store tests use a fixed clock. Three tests use 30-second timeouts.

Changes

Graph path safety

Layer / File(s) Summary
Safe decoding and malformed-path coverage
src/daemon/runtime/codebase/query.ts, tests/daemon/runtime/codebase/query.test.ts
parsePath() catches URI decoding errors and preserves the raw remainder. Tests verify that handleGraphVfs() does not throw for malformed percent escapes.

Test stability

Layer / File(s) Summary
Fixed clocks for log-store tests
tests/daemon/runtime/logs/log-store.test.ts
Persistence, schema, and database-path tests open log stores with a fixed June 20, 2026 clock.
Extended test execution timeouts
tests/cli/daemon-service.test.ts, tests/daemon/runtime/pipeline/memory-redrive.test.ts, tests/cli/health-probes.test.ts
The PowerShell probe, terminal-job reader, and health evaluation tests use 30-second timeouts.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: copilot

Poem

A rabbit guards each graph path,
Safe decoding stops the crash.
Fixed clocks keep records clear,
Longer waits keep tests sincere,
And every check completes.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: safe decoding of graph VFS paths to prevent malformed percent escapes from throwing.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@Diwak4r

Diwak4r commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the CI failures — all three failing jobs (Quality gate Node 22.x, Quality gate Node 24.x, Windows smoke) fail on the exact same, unrelated test:

FAIL tests/daemon/runtime/logs/log-store.test.ts
  > PRD-043a openLogStore persists to disk
  > AC-1: records written before a restart are queryable after re-opening the same logs.db
AssertionError: expected [] to have a length of 3 but got +0

This test is about openLogStore persisting request records to an on-disk logs.db and reading them back after a reopen. It is unrelated to this PR. This PR only modifies:

  • src/daemon/runtime/codebase/query.ts — wraps the graph-VFS path %-decode in a safe fallback (the never-throws contract for handleGraphVfs / resolveGraph), so a malformed % escape no longer raises URIError.
  • tests/daemon/runtime/codebase/query.test.ts — adds coverage for the malformed-% case.

Neither file touches the log store, openLogStore, or the .daemon/logs.db persistence path, so this change cannot affect that test. 5207 tests pass; only this one unrelated test fails (it fails identically across all three jobs, pointing at an environment/persistence issue in the log-store test, not the graph-vfs change).

I've left the log-store test alone to avoid scope creep on an unrelated module — happy to help fix/quarantine it separately if the maintainers want, but it shouldn't block this graph-vfs fix. Requesting review/merge on the isolated change.

…e-prune of test records

The AC-1, AC-6, and db-file-existence tests open a `baseDir`-backed store
without a clock, so the startup prune uses the real system clock. As the
test records carry hardcoded timestamps from 2026-06-20 and the default
retention age cap is 30 days, these rows get pruned on re-open once real
time exceeds that window — causing `AC-1` to fail with `expected [] to
have a length of 3 but got +0` on all three CI legs.

Fix: pass `clock: testClock` (frozen at 2026-06-20T00:00:00.000Z) to both
the write and read stores in those three tests, matching the pattern the
existing age-sweep AC-5 test already uses.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 31, 2026 19:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Diwak4r and others added 2 commits August 1, 2026 01:09
Clock-injection commit d377750 already pushed but CI did not
auto-trigger on the fork push. Empty commit to force it.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 31, 2026 19:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings July 31, 2026 20:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

2 participants