Repository navigation
Default to attempting to serve index.html for a path - #4
Merged
Ethan-Arrowood merged 1 commit intoOct 21, 2025
Merged
Conversation
Member
|
Taking a look at this today and will provide my tests and any additional feedback within the next few hours. |
Ethan-Arrowood
approved these changes
Oct 21, 2025
Member
There was a problem hiding this comment.
Okay I am in favor of this change.
The static plugin continues to serve 404 Not Found responses if the index.html doesn't exist as expected.
Reviewing the source code, the index option really only matters for executing staticFile = indexEntries.get(req.pathname). Regardless if its set or not, the indexEntries map is populated (this way if the user changes the option during runtime index files will start being served automatically without having to reprocess the files glob.
Nice work! I'll merge this after the integration tests run (they should pass without issue as I don't think we have static plugin tests yet). Congratulations on being the first external contributor to Harper 🚀 🐶
heskew
added a commit
that referenced
this pull request
Apr 24, 2026
Stacked onto #402 in response to the deep external review. Changes: Accepted and fixed: - #3 (ai-review-log log step): Ported verbatim from oauth. Harper's reviews now feed the central calibration tracker that the weekly sweep runs against. Adds AI_REVIEW_LOG_TOKEN to the secrets prerequisite list (flagged in PR body update). - #4 (dead `documentation/**` glob): Harper has no `documentation/` dir — the docs site is a separate repo. Replaced with realistic Harper doc-file names (README.md, CLAUDE.md, AGENTS.md, dependencies.md) + package.json keyword edits. Same fix in both mention and issue-to-pr prompts. - #5 (prefix-match label too permissive): `startsWith('claude-fix:')` matched typoed variants (`claude-fix:typos`, etc). Tightened to explicit whitelist of the four supported labels. - #6 (fixed heredoc marker collision risk): Replaced `CLAUDE_SCOPE_EOF` with a random `EOF_$(openssl rand -hex 16)` delimiter. Collision-proof against any content a future ai-review-prompts layer might include. - #7a (eager `npm ci` on mention): Removed. Most mentions (explain, review, small edits) don't need deps — install is ~35-60s × every mention. Prompt now tells the agent to run `npm ci` itself before any script that requires dependencies. issue-to-pr keeps its eager install since that workflow almost always builds/tests. - #8a (Opus cost on every mention): Shifted to Sonnet default with Opus opt-in via case-insensitive word-boundary `deep` in the comment. "Needs deep review of the whole migration" escalates; "fix this typo" stays on Sonnet. Cost gets spent deliberately, not by default. - #9 (no scope-to-diff guidance): Review prompt now tells the agent to start from `git diff --name-only <base>...HEAD` and only expand scope when a specific finding demands it. On a ~1000-file repo this matters. Plus a mention-parsing step that enforces: - `@claude` must be the first non-whitespace token (word-boundary after) — rules out `@claudette`, inline prose mentions, and quoted replies (`> @claude ...`) where the reply addresses a human. The existing `contains('@claude')` job-level `if:` stays as a cheap pre-filter; the new shell step is the real precision gate. - Subsequent steps guard on `steps.mention.outputs.proceed == 'true'`. Comment sharpening (accept the tradeoff, tighten the rationale): - #1 (postinstall RCE via package.json edit): The allowlist comment on both agent workflows previously implied `Bash(npm install)` (no-arg) was a real mitigation. It blocks `npm install @attacker/x` but NOT the `postinstall` path — an injection can edit package.json and then bare `npm install` executes the hostile lifecycle script with GITHUB_TOKEN + the claude[bot] installation token in env. Comment now names this path explicitly. The actual guardrails are branch protection + the author_association gate; a structural fix (`.npmrc ignore-scripts=true`, or dropping `Bash(npm install)` entirely in favor of a separate CI install job) deserves its own PR. - #2 (`Bash(git:*)` contradicts review.yml's stated principle): review.yml's comment previously read as universal guidance. It's actually specific to the read-only review workflow. Comment now explicitly notes that the authoring workflows deliberately grant broader git access and rely on branch protection as the guardrail. Not addressed here: - Splitting issue-to-pr into read-only research + narrow-write commit steps (post-v0.1.0 follow-up). - Tightening mention/issue-to-pr to specific read-only + commit/push git subcommands (same structural PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
May 5, 2026
Stacked onto #402 in response to the deep external review. Changes: Accepted and fixed: - #3 (ai-review-log log step): Ported verbatim from oauth. Harper's reviews now feed the central calibration tracker that the weekly sweep runs against. Adds AI_REVIEW_LOG_TOKEN to the secrets prerequisite list (flagged in PR body update). - #4 (dead `documentation/**` glob): Harper has no `documentation/` dir — the docs site is a separate repo. Replaced with realistic Harper doc-file names (README.md, CLAUDE.md, AGENTS.md, dependencies.md) + package.json keyword edits. Same fix in both mention and issue-to-pr prompts. - #5 (prefix-match label too permissive): `startsWith('claude-fix:')` matched typoed variants (`claude-fix:typos`, etc). Tightened to explicit whitelist of the four supported labels. - #6 (fixed heredoc marker collision risk): Replaced `CLAUDE_SCOPE_EOF` with a random `EOF_$(openssl rand -hex 16)` delimiter. Collision-proof against any content a future ai-review-prompts layer might include. - #7a (eager `npm ci` on mention): Removed. Most mentions (explain, review, small edits) don't need deps — install is ~35-60s × every mention. Prompt now tells the agent to run `npm ci` itself before any script that requires dependencies. issue-to-pr keeps its eager install since that workflow almost always builds/tests. - #8a (Opus cost on every mention): Shifted to Sonnet default with Opus opt-in via case-insensitive word-boundary `deep` in the comment. "Needs deep review of the whole migration" escalates; "fix this typo" stays on Sonnet. Cost gets spent deliberately, not by default. - #9 (no scope-to-diff guidance): Review prompt now tells the agent to start from `git diff --name-only <base>...HEAD` and only expand scope when a specific finding demands it. On a ~1000-file repo this matters. Plus a mention-parsing step that enforces: - `@claude` must be the first non-whitespace token (word-boundary after) — rules out `@claudette`, inline prose mentions, and quoted replies (`> @claude ...`) where the reply addresses a human. The existing `contains('@claude')` job-level `if:` stays as a cheap pre-filter; the new shell step is the real precision gate. - Subsequent steps guard on `steps.mention.outputs.proceed == 'true'`. Comment sharpening (accept the tradeoff, tighten the rationale): - #1 (postinstall RCE via package.json edit): The allowlist comment on both agent workflows previously implied `Bash(npm install)` (no-arg) was a real mitigation. It blocks `npm install @attacker/x` but NOT the `postinstall` path — an injection can edit package.json and then bare `npm install` executes the hostile lifecycle script with GITHUB_TOKEN + the claude[bot] installation token in env. Comment now names this path explicitly. The actual guardrails are branch protection + the author_association gate; a structural fix (`.npmrc ignore-scripts=true`, or dropping `Bash(npm install)` entirely in favor of a separate CI install job) deserves its own PR. - #2 (`Bash(git:*)` contradicts review.yml's stated principle): review.yml's comment previously read as universal guidance. It's actually specific to the read-only review workflow. Comment now explicitly notes that the authoring workflows deliberately grant broader git access and rely on branch protection as the guardrail. Not addressed here: - Splitting issue-to-pr into read-only research + narrow-write commit steps (post-v0.1.0 follow-up). - Tightening mention/issue-to-pr to specific read-only + commit/push git subcommands (same structural PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 12, 2026
This was referenced May 19, 2026
9 of 16 tasks
heskew
added a commit
that referenced
this pull request
Jun 10, 2026
…, thresholds, reindex Four tests through the full stack (real RocksDB, real Table.search(), schema-driven HNSW deployment via component API) guard each of the six data-integrity fixes in commit 251e5b7: 1. delete-entry-point: 50 records, bulk-delete 40 including EP, all survivors reachable. Guards fix #2 (EP replacement scan + transaction + skip-deleted). 2. update-churn: 30 records × 5 re-embed rounds, all records still findable by their final vector. Guards fix #3 (UPDATE sweeps only level l). 3. threshold queries: 2-D vectors at known exact cosine distances verify that le(boundary) is inclusive and lt(boundary) is exclusive. Guards fix #6b (le comparator uses <= not <). 4. reindex backfill: populate table without HNSW, add index, poll until search works, assert all 40 pre-existing records reachable; post-backfill update and delete must behave correctly. Guards fixes #4 and #5. Interrupted-backfill-then-restart is explicitly deferred: 40-record backfill completes in milliseconds so a SIGKILL race would be non-deterministic. That scenario is covered by the unit tests in unitTests/resources/vectorIndex.test.js. Vector search is exercised via the HTTP QUERY method so the body reaches Table.search() without the mapCondition stripping done by search_by_conditions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kriszyp
pushed a commit
that referenced
this pull request
Sep 1, 2026
…e) (#2147) * feat(rest): total-count pagination via `Prefer: count=` (Content-Range) Adds opt-in total-record-count for REST collection queries so a client can paginate ("1–25 of 1,234") without a second round-trip or a custom resource. - `Prefer: count=exact` — Table.search drains the full matched set once, windowing the requested page in the same pass (O(matched) filter evals, O(limit) memory), bounded by MAX_EXACT_COUNT_SCAN so a page fetch can't turn into an unbounded scan. - `Prefer: count=estimated` — returns just the page plus a cheap planner/table estimate (estimateCondition / estimatedEntryCount, now exported), no full scan. - No default: without the header nothing is computed and no header is emitted. - REST emits `Content-Range: items <start>-<end>/<total>` (200, not 206), `Range-Unit: items`, and `Preference-Applied: count=exact|estimated|none`, and adds them to `Access-Control-Expose-Headers` so browser (CORS) clients can read them. HEAD returns the headers with no body — a cheap "how many match?" pre-flight. Tests: resources-level unit (exact/estimated/window/filtered/default streaming) and REST integration (Content-Range/Range-Unit/Preference-Applied/CORS/HEAD/opt-in). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rest): address code-review findings on count pagination Cross-model review (Codex) of the count feature surfaced several correctness, resource, and disclosure issues, all fixed here: - Read-txn leak: the count drain now releases the read transaction in a `finally`, so a throw mid-iteration (record load, rowFilter policy error) can't leak a pinned snapshot. - Guardrail no longer truncates the page: the requested [offset, end) window is always collected in full; the row cap only abandons the running total. Added a wall-clock budget (MAX_EXACT_COUNT_MS) alongside the row cap so an exact count of a large match set can't run unbounded — on exhaustion the total is reported unknown (Content-Range .../*), never a short page. - Estimated totals no longer corrupted by the planner's synthetic `sort` pseudo-condition: hasUserConditions now reads the raw request conditions, and the estimate drops `sort` pseudo-conditions. A clamp keeps a non-empty page's Content-Range valid when an estimate undershoots (exact totals stay authoritative). - Estimated totals return unknown (null -> .../*) when an opaque rowFilter/vectorFilter participates, instead of a misleading estimate that could disclose hidden cardinality. - Spurious headers: the REST gate now requires an array result, so a single-record GET whose record carries a `recordCount` attribute can't be mistaken for a count page. - CORS: Access-Control-Expose-Headers is appended (not overwritten), preserving a resource's own exposed headers. Adds regression tests for the sorted-estimate, filter-aware estimate, and filtered-exact paths. Resources unit 8 passing; REST integration 21 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(rest): per-mount `exactCount` config gate for count=exact Adds an operator control for the expensive exact-count scan: `rest: { exactCount: false }` on a REST mount serves a `Prefer: count=exact` request as a cheap estimate instead (signaled back via `Preference-Applied: count=estimated`), rather than rejecting it. Default enabled. Read from httpOptions in the same per-mount way as the existing `includeExpensiveRecordCountEstimates` option. This is the operator-facing half of the DoS mitigation for exact counts: the in-code guardrails (row cap + time budget) bound a single request, and this lets a deployment turn exact counts off entirely on a sensitive/public mount. It is a per-REST-mount policy — components exporting at the shared root path share one mount's options. Integration: a dedicated suite (its own instance, since a gated component would otherwise share the root mount with the main suite) verifies count=exact downgrades to estimated while count=estimated is unchanged. 23 REST integration tests passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(rest): guard count-page Bytes against read-buffer aliasing A code-review concern held that the count path releases its read transaction before the page is serialized, so a Bytes/Blob field decoded as a zero-copy view of the read buffer could be corrupted by later reads/writes. Verified it does NOT occur: the count drain reads every record eagerly while the txn is open and returns owned copies (Bytes come back as standalone Buffers, byteOffset 0), so releasing before serialize is safe — unlike the streaming path, which reads lazily during serialization and must hold the txn. This test churns writes/reads after an exact count and asserts the returned Bytes are unchanged, on both storage engines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rest): echo requested count mode on unavailable totals; robust exactCount gate Addresses two review findings: - #2: Preference-Applied now echoes the count mode the server applied (exact|estimated, after any per-mount downgrade) instead of `count=none` when the total is unavailable. A `Content-Range: items x-y/*` now reads as "that mode was applied but the total is unavailable" (guardrail hit, or an estimate suppressed by an opaque filter / Infinity estimate) rather than "no count was requested". Added an integration case: a `ne` condition (Infinity estimate) yields items 0-.../* with count=estimated. - #4: the exactCount disable check also accepts the string "false", since not every config source coerces to a boolean. 24 REST integration tests passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Formatting * fix(rest): require a limit for count so the guardrail can't be bypassed Review (claude[bot] on #2147) found the exact-count guardrail (row cap + time budget) and the estimated early-exit only applied when the request included a limit(): both live inside `if (end !== undefined ...)`. A count=exact/estimated request with no limit() therefore drained AND materialized the entire matched set with no cap — the exact unbounded-scan/-memory DoS the guardrail was built to prevent, on the most likely-hit path (a bare collection GET), and it bypassed the exactCount gate too. Counting is a pagination feature, so it now requires a limit(): a count request without one falls through to the normal streaming path (no count emitted), which keeps the guardrail always applied to a bounded page. Updated the unit test that documented the no-limit drain as intentional, and added a test asserting a no-limit count streams (does not materialize). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: prettier format queryCount test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rest): address review — bound count pages, opt-in exact, GET/HEAD only, Vary/CORS Addresses kriszyp's review on #2147: - Bound the count page: the count path now requires a finite, non-negative integer limit no larger than MAX_COUNT_PAGE (10k). limit(Infinity), limit(foo)->NaN, a negative, or an oversized limit fall through to streaming with no count, so a count request can't be coerced into materializing an unbounded page. - Exact counting is now opt-in per mount (`rest: { exactCount: true }`, default off); count=exact is otherwise served as an estimate. Estimated stays the safe default, removing the default worker-saturation surface on public tables. - Only honor Prefer: count on GET/HEAD. It was set for every method, so a collection DELETE carrying limit()+Prefer received a materialized array from search() (declared AsyncIterable) and threw instead of deleting. - Emit `Vary: Prefer` on collection reads (after serialize, which resets Vary) so a shared cache can't serve count headers to a request that didn't ask, or a cached non-count response to one that did. - Compare Access-Control-Expose-Headers as case-insensitive comma tokens, not substrings, so an unrelated existing token (e.g. X-Content-Range-Metadata) no longer suppresses the real Content-Range token. Tests: unit 11 passing (added invalid/oversized-limit fall-through); integration 27 passing (oversized-limit fall-through, Vary: Prefer, DELETE-not-misrouted, and the new opt-in default via exactCount: true / default-off suites). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rest): validate count offset and total window, not just the limit Follow-up to kriszyp's review on #2147: the bounded-page check validated the limit but accepted any offset. A negative offset (limit(-5,10)) diverged from the normal slice path, and an arbitrarily large offset postponed the exact-count guardrails (which engage only past the page window) until that offset had been scanned. The count path now also requires the offset to be a finite, non-negative integer and the window (offset + limit) to be within MAX_EXACT_COUNT_SCAN; anything else (a negative offset, or a deep-page window past the scan budget) falls through to streaming with no count. Adds unit + integration coverage for negative and oversized-window offsets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(rest): use rocksdb-js estimateCount for range count estimates Bumps @harperfast/rocksdb-js to 2.8.0 and wires its new statistical range key-count estimator (`estimateCount`) into the query planner's range estimates, which the `Prefer: count=estimated` pagination path reads. Range comparators (between, starts_with, greater/less, open ranges) previously returned an arbitrary fixed fraction of the table size, because the storage layer could not estimate a range's cardinality. `estimateCondition` now asks the engine for a real range estimate on RocksDB, and only falls back to the old heuristic when unavailable (LMDB engine, non-indexed / custom-indexed attribute, an unbounded/degenerate range, or a zero-confidence — failed-statistics — read). Equals still uses the exact per-value index count; this only replaces the range guesses. Because the REST estimated-count path funnels through `estimateCondition`, `count=estimated` now reports a range-aware total, and the planner picks indexes for range queries from real selectivity rather than a constant. `RocksIndexStore.estimateCount` overrides the inherited estimator to apply the same `[indexedValue, primaryKey]` composite-key rewrite as its `getRange`, so a secondary-index range estimate covers exactly the keys the scan would visit (otherwise an inclusive end / exclusive start would miss the value's bucket). Tests: resources unit adds primary-key and secondary-index range-estimate cases proving the total tracks range width (a narrow tail estimates fewer than the whole table/index) where the old range-blind heuristic would tie. Existing count, planner, and search suites green (1803 resources tests passing); tsc and lint clean. Behavior on LMDB is unchanged (falls back to the prior heuristic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rest): don't advertise an HNSW/vector-sorted count as exact A `Prefer: count=exact` request sorted by a vector/HNSW attribute (or shaped by a `vectorFilter`) drained `scanned` rows and reported that as the exact total. An HNSW traversal returns a bounded, approximate candidate set whose size is chosen from `minResults` (offset + limit), so `scanned` tracks the requested page size, not the true match count — the same query at limit(5) vs limit(200) could advertise two different `count=exact` totals. The count path now detects an approximate (vector-sorted or vector-filtered) result set and reports the total as unavailable (`recordCount` null, `recordCountExact` false → `Content-Range: items x-y/*`) instead of a page-size-dependent number, mirroring how the estimated branch already bails to null for an opaque row/vector filter. Pages still materialize normally; only the untrustworthy total is withheld. Regression test (`queryCountVector.test.js`): the same cosine-sorted query at limit(5) and limit(40) must report the total unavailable at both, not two different exact numbers. Full resources suite green (1883). Addresses the standing review blocker raised across rounds (2026-08-20..31). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(rest): treat any custom-index (vector) traversal as an approximate count Broadens the approximate-result detection from the previous fix: an HNSW `lt`/`le` threshold filter drives the same bounded, minResults-widened traversal as a vector sort (HierarchicalNavigableSmallWorld handles `lt`/`le` in the same switch as `sort`), and can be the driving condition with no `sort` clause at all — so `count=exact` over it advertised a page-size-dependent `scanned` as authoritative, the same defect the sort fix addressed, reached through a sibling comparator. Detection now walks the executing `conditions` (recursively, through OR groups) for any attribute backed by a custom index, plus the `vectorFilter` check. This is both broader (catches the threshold-filter path) and more precise than the sort-chain walk: a vector sort applied as in-memory post-ordering leaves no custom-index condition in `conditions`, so it correctly stays exact rather than being over-flagged. Regression test adds the `lt` threshold-filter case (no sort) at two page sizes; verified it reports `scanned` as exact under the old sort-only detection and unavailable under this one. Addresses the follow-up review blocker on the HNSW fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(rest): keep the exact-count drain from blocking the event loop; harden Prefer parse Cross-model review (Gemini + Cursor-Grok) findings on the count path: - The exact-count drain iterated the store's async iterator with no macrotask yield. On a store whose iterator settles synchronously (the common indexed-scan case) a large `count=exact` scan ran as one uninterrupted microtask burst, blocking the event loop for up to the whole MAX_EXACT_COUNT_MS budget and starving concurrent requests. Yield to the macrotask queue every COUNT_YIELD_INTERVAL rows so I/O and other requests keep progressing — covering the page-window scan too, not just the tail past it. - For an approximate (vector/HNSW) result set, `count=exact` now stops at the page window instead of draining the tail: the total is reported unavailable anyway, so the tail work produced a number that was never published. - REST Prefer parse hardened against malformed input: optional-chain `httpOptions` (a programmatic mount may pass none) and String()-coerce the preference value before lower-casing (a bare `Prefer: count` with no `=` yields a non-string). Not changed — an unadjudicated Cursor-Grok "async allowRead breaks count" blocker was investigated and did NOT reproduce: driving `get()` with an async allowRead and `Prefer: count=` returns a proper page array (recordCount intact, iterates cleanly); the Table-level async-authorization branch isn't reached for the count path (auth resolves at the Resource layer first). No fix shipped for a non-issue. The 8 pre-existing resources-suite failures (transaction-log/snapshot/audit/reload) reproduce identically on a clean tree and are unrelated to this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Default to serving
index.htmlwhen a static path is not foundSummary
Flip the default behavior for static file handling so that when a requested path does not match a concrete asset, the server attempts to serve
index.htmlif one exists.What changed
scope.options['index']isundefined, it now defaults totrue(previouslyfalse).index.htmlif it exists for the path.index: false.Motivation / Context
://app:port/path/index.htmlwhen playing around with Fabric at JSConf, hopefully this change updates to make it so that://app:port/pathattempts to serveindex.htmlif one exists for that path.Note for reviewers
At the time of this writing, I wasn't able to figure out how to test properly locally, so I wasn't able to test this change. I made this update based on conversations with the team, and chatting about it. But just to be safe, I want to explicitly call out: if this is going to land, please test it and confirm behavior! If I figure out how to test this before it lands, I'll come back and update this part of the description.