Skip to content

docs(resources): fix four static-verb examples that fail silently on Harper 5 - #658

Merged
Ethan-Arrowood merged 4 commits into
mainfrom
fix/static-verb-examples-await-body
Sep 2, 2026
Merged

docs(resources): fix four static-verb examples that fail silently on Harper 5#658
Ethan-Arrowood merged 4 commits into
mainfrom
fix/static-verb-examples-await-body

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 1, 2026

Copy link
Copy Markdown
Member

Four v5 reference examples taught patterns that fail silently on Harper 5 — no throw, no type error, just wrong behavior. Each is also a mode: generate source for a harper-best-practices rule, so the broken code was being shipped to agents.

The shared root cause is documented in harper's own resources/DESIGN.md -> Conventions: overriding a static verb replaces the transactional() wrapper, including the when(data, ...) that resolves the body. A promise has no own enumerable properties, so every field reads undefined rather than raising.

# File Defect Symptom
1 reference/database/schema.md (Blob Usage) super.get(target) not awaited record.data is undefined, so blob.on('error', ...) throws a TypeError on every blob read
2 reference/database/api.md (Accepting Binary in JSON Requests) body read off the unresolved promise the if (record.data) branch never runs and super.post stores the raw base64 string
3 reference/resources/overview.md (Extending a Table) this.create(...) neither awaited nor returned POST responds before the commit; a rejected create becomes an unhandled rejection, which under Node's default --unhandled-rejections=throw kills the worker thread
4 reference/resources/resource-api.md (Resource Static Methods) instance verbs written with the static argument order an unflagged instance post is dispatched as post(data, query), so data.username/data.password are always undefined and context.login() fails every time

What changed

Defects 1-3 are one-line corrections. Defect 4 converts SignIn/SignOut/getCurrentUser to the static form the rest of that page prescribes ("For new code, prefer static methods and omit the flag"), matching the already-correct static async post(_target, data) examples in reference/security/jwt-authentication.md.

The context now arrives as a parameter, not from this.getContext(). getContext() and getCurrentUser() are declared on Resource.prototype only — there is no static counterpart — so this.getContext() inside a static verb is MyClass.getContext, i.e. undefined. server/REST.ts dispatches resource.get(target, request) and resource.post(target, request.data, request) against the class, so a static verb reads the context from its trailing argument. This is exactly how core's own resources/login.ts implements the pre-authentication login endpoint. The getContext() and getCurrentUser() entries, both listed under Resource Static Methods, now say so.

A second defect the pre-push review found

The sign-out example called context.session.delete(context.session.id), and the prose recommended it. That method does not exist at runtime: security/auth.ts:279 attaches only update to request.session, and core's own logout path ends a session with session.update({ user: null }) (security/auth.ts:444). The guard was dead too — security/auth.ts:126 sets request.session to {} rather than leaving it undefined when sessions are enabled and no cookie is present, so if (!context.session) never fired and an unauthenticated POST reached the nonexistent delete for a 500 instead of the documented 401. Both the example and the prose now use context?.session?.user and context.session.update({ user: null }).

Verification

  • npm ci && npm run build — clean (406 documents processed, no broken links), re-run after the guard commit. The two new anchors were checked directly in the build output: #getcontext-context-1 and http/api#properties both resolve to real id= attributes.
  • npm run format:check — clean.
  • Behavior claims traced in a local read-only HarperFast/harper checkout: resources/DESIGN.md -> Conventions (the MaybePromise obligation), resources/Resource.ts:255 (loadAsInstance === false ? resource.post(query, data) : resource.post(data, query)), resources/Resource.ts:516,524 (getContext/getCurrentUser are prototype-only), resources/login.ts (the static sign-in shape), server/REST.ts:242,244 (verb dispatch against the class), security/auth.ts:126,279,444 (session shape and the real logout call).
  • Acceptance grep over reference/database/, reference/resources/ and reference/security/ for super.get( / super.post( / super.put( / super.patch(: 12 call sites, every one now awaited or returned. The only unawaited-and-unreturned one was schema.md:869, fixed here. Every static verb example that touches a request body now awaits it first — api.md:244, overview.md:59, resource-api.md:115,136,155,755, jwt-authentication.md:156,171.
  • No REST-verb instance handler remains in resource-api.md's Resource Static Methods section other than the caching-source examples under sourcedFrom() (class MySource extends Resource { async get() ... }, the write-through put/delete, async post(data) under "Loading from source in write methods", async *subscribe()) — instance methods are the correct and intended form for source resources, so those were deliberately left alone.
  • reference_versioned_docs/** untouched; no heading renamed.

Review feedback

gemini-code-assist opened five nullish-guard threads on the examples above. Every one is reachable on the documented dispatch path — server/REST.ts:215 assigns request.data only when the request carries content-length or transfer-encoding, so a body-less POST really does await to undefined — so all five are addressed. Two of them differ from the literal suggestion:

  • api.md answers a missing body with a 400 rather than optional-chaining past it. if (body?.data) on its own forwards the nullish body to super.post(target, body), which lands in transactional()'s single-argument branch (resources/Resource.ts:589-603) and stores the RequestTarget as the record — the same silent failure this PR exists to remove. The pre-push review caught that on the first attempt at this thread.
  • schema.md guards record?.data and returns the record early. Optional chaining alone would answer a missing record with a 200 and an empty body; a nullish GET return is what REST turns into a 404 (server/REST.ts:283), and the sibling Serving Binary from a Resource example in api.md already reads exactly this way.

The other three take the suggestion:

  • (await data) ?? {} in the sign-in example — verbatim what core's own static sign-in does (resources/login.ts). Without it an empty POST to a sign-in endpoint is a destructuring TypeError (500) instead of a 403.
  • context?.user and context?.session?.user — a static verb called from server-side code receives a context argument only if the caller passes one, which is what the getContext() entry above them now says. Both guards already returned 401 for "no authenticated user", which is the honest answer when there is no context either.

Downstream

HarperFast/skills#79 reported these against the published harper-best-practices skill. The four affected rules — using-blob-datatype, handling-binary-data, extending-tables, checking-authentication — are mode: generate in rules.manifest.yaml, generated from these exact files and sections. The chain is: this PR merges -> auto/docs-sync regenerates harper-best-practices -> first-party apps refresh their vendored copy with npx skills update --project. Patching the skills repo or the vendored copies directly would be reverted by the next regeneration, which is why the fix belongs here. HarperFast/skills#79 can be closed once the regenerated rules land.

For the human reviewer

  • The judgment call, and why it went this way. Defect 4's examples could instead have been kept as instance verbs by adding static loadAsInstance = false;. Converting won because the page itself says "For new code, prefer static methods and omit the flag", the sibling jwt-authentication.md examples are already static, and the rest of the skill corpus prescribes static.
  • getCurrentUser()'s example no longer calls getCurrentUser(). It cannot: the section sits under Resource Static Methods, and the method is prototype-only. The example is now a static verb reading context.user, with a sentence saying getCurrentUser() is the instance-side accessor. If you would rather the section keep demonstrating its own method, the alternative is to move it — but headings feed the skills manifest's section: selectors, so a move is not free.
  • Declined, out of scope — worth separate issues. The pre-push review flagged four pre-existing items outside the four reported defects:
    1. overview.md:59 — a static post override means no allowCreate predicate runs (resources/DESIGN.md: "the override makes the access-control decision"), and the extending-a-table example does not say so. True before this PR too, and probably the most valuable follow-up here.
    2. resource-api.md:157this.create(target, data.content) mis-orders Resource.create's arguments. With loadAsInstance unset, Resource.ts:214-225 takes the two-argument form and shifts, so the RequestTarget is stored as the record and data.content becomes the context. Same silent-failure class, different mechanism. I traced this one; it is real.
    3. resource-api.md:1152static async function get(target) is invalid class syntax.
    4. reference/components/javascript-environment.md:89 — still recommends this.getCurrentUser() in a Resource method without the static-dispatch caveat this PR adds.
  • Disagreed with one finding. The review suggested resource-api.md:69's non-async static get(target) { ... return super.get(target); } can resolve to a pending promise. It cannot: server/REST.ts awaits the dispatch result (await transaction(request, ...), and transaction() forwards a thenable), and super.get is a MaybePromise by design. Left as is.
  • Declined a nit. The review called the comment on overview.md:58 narration. It states the consequence of returning the promise, which is the defect being fixed and feeds the generated extending-tables rule, so it stays.
  • Coverage gap. The Gemini leg has failed auth on every round in this worktree (agy not authenticated). Cross-model coverage was codex + cursor-composer on rounds 1 and 4, codex alone on the round-2 and round-3 deltas. The round-4 domain-adjudication leg timed out, so the findings below are unadjudicated. Round 4 confirmed the 400 guard resolves the finding it raised in round 3 and produced no new ones; cursor-composer reported no blockers, concerns, or suggestions.

Refs HarperFast/skills#79

🤖 Generated with Claude Code

https://claude.ai/code/session_019drMJUhfEQfeNVDcU22oNe

Complexity: easy

Review-Coverage: authored=claude; ran=codex,cursor-composer; blocked=gemini(auth),domain(timeout); declined=cursor-grok; rounds=4 @ a15776b

Human-Review-Need: 4 @ a15776b

kriszyp and others added 2 commits September 1, 2026 15:41
Four v5 reference examples taught patterns that fail silently on Harper 5.
Overriding a static verb replaces the transactional() wrapper, so the record
or body arrives as an unresolved promise and every field reads undefined
instead of raising (harper resources/DESIGN.md -> Conventions).

- database/schema.md blob error handling: `super.get(target)` was not awaited,
  so `record.data` was undefined and `blob.on('error', ...)` threw on every
  read. The equivalent example in database/api.md already awaits it.
- database/api.md base64-in-JSON: the `if (record.data)` branch never ran, so
  `super.post` stored the raw base64 string. Await the body once and forward it.
- resources/overview.md extending a table: `this.create(...)` was neither
  awaited nor returned, so POST responded before the commit and a rejected
  create became an unhandled rejection.
- resources/resource-api.md sign-in/sign-out and getCurrentUser: instance verbs
  written with the static argument order. An unflagged instance `post` is
  dispatched as `post(data, query)` (harper resources/Resource.ts), so
  `data.username` was always undefined. Converted to the static form the rest
  of the page prescribes, taking the context as a parameter — `getContext()`
  and `getCurrentUser()` are instance methods with no static counterpart, so
  the context arrives as `(target, context)` / `(target, data, context)`,
  matching core's own login.ts and security/jwt-authentication.md.

Refs HarperFast/skills#79

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019drMJUhfEQfeNVDcU22oNe
Independent pre-push review found a second defect in the same block.
`request.session` carries only `update` (harper security/auth.ts:279); there
is no runtime `delete`, and core's own logout ends a session with
`session.update({ user: null })` (security/auth.ts:444). The guard was also
dead: `request.session` is set to `{}` rather than left undefined when
sessions are enabled and no cookie is present (security/auth.ts:126), so
`!context.session` never fired and an unauthenticated POST reached the
nonexistent `delete` for a 500 instead of the documented 401.

Also narrows the static-context note: a direct server-side call supplies a
context argument only if the caller passes one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019drMJUhfEQfeNVDcU22oNe

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates documentation and code examples across several markdown files to correctly handle asynchronous operations, static resource methods, and request contexts. Specifically, it clarifies how to await request bodies, handle static verbs, and manage user sessions. The review feedback highlights several opportunities to make the example code snippets more robust by using optional chaining and fallback values to prevent potential runtime TypeErrors when parameters like body, record, or context are missing or undefined.

Comment thread reference/database/api.md
Comment thread reference/database/schema.md
Comment thread reference/resources/resource-api.md Outdated
Comment thread reference/resources/resource-api.md Outdated
Comment thread reference/resources/resource-api.md Outdated
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-658

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-658 September 1, 2026 22:05 Inactive
@kriszyp
kriszyp marked this pull request as ready for review September 2, 2026 04:45
@kriszyp
kriszyp requested a review from a team as a code owner September 2, 2026 04:45
@cb1kenobi

Copy link
Copy Markdown
Member

Reviewed 8db0609 and found no blocking issues. No confirmed blocking findings remain on the changed lines. The fixes correctly await asynchronous values and use the documented static resource context and session APIs.


Generated by Barber AI

… examples

Review feedback on the five examples this PR already touches. Each guard covers
a state the documented dispatch path can actually reach — `server/REST.ts:215`
assigns `request.data` only when the request carries `content-length` or
`transfer-encoding`, so a body-less POST really does await to `undefined`.

- api.md answers a missing body with a 400 rather than optional-chaining past
  it. Forwarding a nullish body to `super.post(target, body)` would land in
  `transactional()`'s single-argument branch (`resources/Resource.ts:589-603`)
  and store the `RequestTarget` as the record — the same silent failure this PR
  exists to remove.
- schema.md guards the missing record and returns it early. Optional chaining
  alone would answer a missing record with a 200 and an empty body; a nullish
  GET return is what REST turns into a 404 (`server/REST.ts:283`). The sibling
  "Serving Binary from a Resource" example in api.md already reads this way.
- `(await data) ?? {}` (resource-api.md) is verbatim what core's own static
  sign-in does (`resources/login.ts`), and without it an empty POST to the
  sign-in endpoint is a destructuring TypeError instead of a 403.
- `context?.` in the `getCurrentUser` and `SignOut` examples — a static verb
  called from server-side code gets a context only if the caller passes one,
  as the `getContext()` entry above them already explains. Both guards were
  already returning 401 for "no authenticated user", which is the honest
  answer when there is no context either.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-658

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-658 September 2, 2026 05:10 Inactive
@Ethan-Arrowood
Ethan-Arrowood enabled auto-merge (squash) September 2, 2026 19:22
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🚀 Preview Deployment

Your preview deployment is ready!

🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-658

This preview will update automatically when you push new commits.

@github-actions
github-actions Bot temporarily deployed to pr-658 September 2, 2026 19:25 Inactive
@Ethan-Arrowood
Ethan-Arrowood merged commit 5358de5 into main Sep 2, 2026
10 checks passed
@Ethan-Arrowood
Ethan-Arrowood deleted the fix/static-verb-examples-await-body branch September 2, 2026 19:28
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🧹 Preview Cleanup

The preview deployment for this PR has been removed.

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.

3 participants