Skip to content

docs: fixes from skills#81 - #662

Merged
Ethan-Arrowood merged 1 commit into
mainfrom
docs/fixes-from-skills-81
Sep 2, 2026
Merged

docs: fixes from skills#81#662
Ethan-Arrowood merged 1 commit into
mainfrom
docs/fixes-from-skills-81

Conversation

@Ethan-Arrowood

@Ethan-Arrowood Ethan-Arrowood commented Sep 2, 2026

Copy link
Copy Markdown
Member

Five items were raised against v5 reference content while reviewing HarperFast/skills#81. Four were real docs-source errors and are fixed here; one was a reviewer mistake and is left alone with the evidence below.

Every behavior claim was traced in a read-only HarperFast/harper checkout at origin/main (d1eae3d7f).

1. reference/database/api.mdtransaction() timer example did not await

Wrong: the background-job example under transaction(context?, callback) -> Basic Usage called transaction(...) bare. transaction() returns a promise that resolves after commit, so the timer callback finished before observing the result: a rejected write became an unhandled rejection, and a later tick could overlap a still-running transaction.

Two further defects in the same snippet: neither transaction nor isMainThread was imported, so the example throws ReferenceError as written.

Verified against: resources/transaction.ts:87 (_assignPackageExport('transaction', transaction) — it is a harper export, matching reference/resources/resource-api.md:1216); isMainThread has no harper export (the full export list is Resource, contentTypes, createBlob, databases, logger, models, operation, secrets, server, tables, threads, transaction), so it comes from node:worker_threads as it does in resources/replayLogs.ts:7.

Changed: the call is awaited, both imports added, and the body wrapped in try/catch/finally with a running flag. The catch is load-bearing, not decoration: setInterval ignores the promise its async callback returns, so awaiting alone relocates the unhandled rejection rather than removing it. Two short bullets below the snippet state both rules.

2. reference/security/jwt-authentication.md — scoped-token example showed no authentication

Wrong: two connected problems. ## Create Authentication Tokens said flatly "No Authorization header is required for this operation", which holds only for the username/password shape. ## Scoped Tokens (Inline Role) then asserted the request "must be authenticated as a super_user" while showing a bare JSON body with no authentication anywhere — so a request copied from the page fails with a 403.

Verified against:

  • security/impersonation.ts:119-121if (!trusted && !minter?.role?.permission?.super_user) throw new ClientError('Only super_user can create a token with an inline role', 403);. The super_user requirement is real, and the error string in the doc is quoted from here.
  • security/tokenAuthentication.ts:288-290'password' cannot be combined with an inline 'role' object, confirming the payload cannot carry its own credentials.
  • security/tokenAuthentication.ts:152-153 (comment) — "create_authentication_tokens is NO_AUTH, so verifyPerms ... never runs here", and utility/operation_authorization.ts:470 lists it as NO_AUTH. So the minter identity comes from whatever the auth handler resolved, not from an operation permission check.
  • integrationTests/apiTests/token-auth.test.mjs:198 — the passing test is named "scoped token: super_user mints an inline-role token for a non-existent username" and issues the mint through client.req(), which sets Authorization: Basic <base64 user:pass> for the admin super_user (integrationTests/apiTests/utils/client.mjs:9). Basic Auth is the mechanism the test itself uses.
  • unitTests/security/impersonation.test.js:659 — "untrusted mint without a super_user minter is rejected with 403".
  • server/serverHelpers/serverUtilities.ts:371-378server.operation(op, context, authorize) assigns operation.hdb_user = context?.user and sets bypassAuth = !authorize, so authorize: true is what makes the mint run as (and be checked against) the calling user.

Changed: (a) the no-header claim is scoped to the body-credential shape, with the other two shapes named; (b) a cURL example with an Authorization: Basic header was added, plus the exact 403 text and the server.operation(..., context, true) in-component alternative.

3. reference/security/jwt-authentication.mdIssueTokens static-verb signature: no change, the reviewer is wrong

The automated reviewer claimed Harper statics receive (target, data) and that the request context must come from getContext() rather than a second parameter, making static async get(_target, context) wrong.

That is not how get is dispatched. Verb signatures differ by whether the verb has a body:

  • server/REST.ts:298resource.get(target, request)
  • server/REST.ts:304resource.delete(target, request)
  • server/REST.ts:300,302,306resource.post/put/patch(target, request.data, request)

Resource.ts is explicit that the two-argument form for a body-less verb is (id, context):

} else if (hasContent === false) {
    // (id, context), preferred form used for methods that are explicitly without a body
    context = (dataOrContext as any).getContext?.() || dataOrContext;
}

(resources/Resource.ts:651-653, reached because static get is declared with hasContent: falseresources/Resource.ts:113 and :138; static delete likewise at :186,190.)

So static async get(_target, context) is correct — in fact it is the form Resource.ts calls preferred. The sibling static async post(_target, data) examples are correct too, and already await data, consistent with resources/DESIGN.md -> Conventions ("data is a MaybePromiseawait it"). RefreshJWT shares no flaw. Nothing was changed in either example.

For completeness, the reviewer's alternative is not itself imaginary: getContext is exported from harper (index.ts:12). It is the right tool for a static that must also work when called directly from server-side code with no context argument. It is not a correction to the REST-dispatched example on this page.

4. reference/rest/overview.mdrest: true requirement contradicted its own carve-out

Wrong: the numbered requirement and the "Neither half is sufficient on its own" paragraph in ## Tables and Their Automatic Endpoints presented @export and rest: true as symmetric hard requirements, while the :::note a few lines down correctly documents that a component directory with no configuration file gets REST from Harper's built-in default.

Verified against: components/DEFAULT_CONFIG.ts:1-2 sets rest: true in the built-in default; components/componentLoader.ts:638-650 picks harper-config.yaml, then harperdb-config.yaml, then (non-root) config.yaml, and only falls back to DEFAULT_CONFIG when none exists — the file is parsed and used verbatim with no merge against the default. That disjointness is exactly why the carve-out is all-or-nothing and why an existing config.yaml omitting rest turns REST off.

Changed: @export is now stated as always required; rest: true is stated as required whenever a configuration file exists. The "Neither half is sufficient" paragraph now ends by pointing at the no-configuration-file case as the sole exception, so the requirement and the note agree.

5. reference/components/applications.md — ref constraint contradicted the commit-SHA bullet

Wrong: under ## Remote Management -> Deploying by Reference, the fetchable-ref paragraph read as an absolute rule ("A ref must also name something a clone can fetch: refs/heads/* and refs/tags/*, or a bare branch or tag name. Anything else ... is rejected"), which tells a reader that a SHA-based rollback is invalid — contradicting the paragraph above it.

Verified against bin/cliOperations.ts, which shows the constraint is narrower than the docs implied:

  • assertCloneableRefNamespace (:382) returns early unless the ref starts with refs/: if (!ref.startsWith('refs/') || ref.startsWith('refs/heads/') || ref.startsWith('refs/tags/')) return;. The namespace rejection only ever applies to a refs/-qualified ref. Its own comment says as much: "It can't catch a bare SHA that happens to be unreachable: an object ID carries no namespace to inspect."
  • resolveExplicitRef (:428-441) then tries git rev-parse --verify <ref>^{commit} locally, and on failure if (FULL_OBJECT_ID.test(ref)) return ref; // already immutable; nothing to pin it to, where FULL_OBJECT_ID = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/i (:323) covers SHA-1 and SHA-256 object IDs. Only after that does it fall back to ls-remote for refs/tags/<ref> / refs/heads/<ref>.

Changed: the paragraph now leads with the exception — a full commit SHA is accepted directly with no resolution attempted — then states the constraint as applying to every other form, with the namespace rejection correctly scoped to qualified refs.

Exclusions honored

Verification

  • npm run format:write then npm run format:check — clean.
  • npm run build — succeeds, 406 documents processed, zero broken-anchor warnings (the only [WARNING] line is the pre-existing docusaurus-plugin-llms-txt route-exclusion notice, present on main).
  • Both anchors in new links were confirmed against the generated HTML rather than trusted to the build's exit code: id="scoped-tokens-inline-role" in build/reference/v5/security/jwt-authentication.html and id="serveroperationoperation-context-authorize" in build/reference/v5/http/api.html, each matching the emitted href.
  • reference_versioned_docs/ untouched; no heading renamed, so no skills-manifest section: selector moves.

Refs HarperFast/skills#81

🤖 Generated with Claude Code

Each of these is a real error in v5 reference content that a `mode: generate`
skill rule reproduced faithfully, so agents were being taught the broken
pattern.

- `database/api.md` — the `transaction()` timer example did not await the
  transaction, so a failed write became an unhandled rejection the callback
  never observed. Awaited, wrapped in try/catch/finally, and given an overlap
  guard for jobs that can outlast their interval. Also added the missing
  `transaction` and `isMainThread` imports.
- `security/jwt-authentication.md` — the "no `Authorization` header is
  required" claim is true only for the username/password flow; it is now
  scoped to that shape. The scoped-token example showed no authentication
  even though the mint requires an authenticated super_user, so it now shows
  Basic Auth and names the `server.operation(..., true)` alternative.
- `rest/overview.md` — the `rest: true` requirement contradicted its own
  no-configuration-file carve-out. `@export` is always required; `rest: true`
  is required whenever a `config.yaml` exists.
- `components/applications.md` — the fetchable-ref constraint read as an
  absolute rule and so implied that valid SHA-based rollbacks are rejected.
  The full-commit-SHA exception is now stated in that paragraph.

Refs HarperFast/skills#81

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Ethan-Arrowood
Ethan-Arrowood requested a review from a team as a code owner September 2, 2026 14:29

@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 the documentation across several files, clarifying commit SHA deployments, improving the transaction basic usage example with error handling and concurrency guards, and detailing REST configuration and JWT authentication flows. The review feedback highlights a missing logger import in the database API example and suggests splitting combined sentences to make critical security requirements and fallback behaviors more prominent and readable.

Comment thread reference/database/api.md
Comment on lines +107 to +108
import { isMainThread } from 'node:worker_threads';
import { tables, transaction } from 'harper';

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.

high

The code example uses logger.error in the catch block, but logger is not imported from 'harper'. Since logger is a package export of 'harper', omitting it will result in a ReferenceError at runtime when an error is caught. Please add logger to the imported members.

Suggested change
import { isMainThread } from 'node:worker_threads';
import { tables, transaction } from 'harper';
import { isMainThread } from 'node:worker_threads';
import { logger, tables, transaction } from 'harper';

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.

logger is available in the globals, though

## Create Authentication Tokens

Call `create_authentication_tokens` with your Harper credentials. No `Authorization` header is required for this operation.
Call `create_authentication_tokens` with your Harper credentials. When the request carries a `username` and `password` in the body, no `Authorization` header is required — the operation authenticates from the body. Other shapes of this operation do need an authenticated caller: sending no credentials at all mints tokens for the user the request is already authenticated as, and [minting a scoped token](#scoped-tokens-inline-role) requires an authenticated `super_user`.

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.

medium

The fallback behavior regarding sending no credentials is combined with other concepts in a single sentence. To ensure readers scanning the documentation can easily find it, please present this fallback behavior in separate, distinct sentences.

For example:

Call `create_authentication_tokens` with your Harper credentials. When the request carries a `username` and `password` in the body, no `Authorization` header is required — the operation authenticates from the body. Other shapes of this operation do need an authenticated caller. Sending no credentials at all mints tokens for the user the request is already authenticated as. Minting a scoped token (#scoped-tokens-inline-role) requires an authenticated `super_user`.
References
  1. Ensure critical security warnings, such as unauthorized access risks or fallback behaviors, are presented in separate, distinct sentences rather than being combined with other concepts or buried behind semicolons, so that readers scanning the documentation can easily find them.

A super user can mint a **scoped token**: a single JWT whose permissions are embedded in the token itself, so the bearer needs no pre-existing user or role record. This is useful for handing a limited credential (for example, read-only access) to an external service or script without provisioning it in `hdb_user`.

Pass `role` as an inline role-shaped object (the same `permission` structure used by [`add_role`](../users-and-roles/overview.md), including the `operations` allowlist). The request must be authenticated as a `super_user`; no `password` may be included:
Pass `role` as an inline role-shaped object (the same `permission` structure used by [`add_role`](../users-and-roles/overview.md), including the `operations` allowlist). Unlike the username/password flow above, this shape carries no credentials of its own — the **minter** must be authenticated as a `super_user`, and no `password` may be included in the body:

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.

medium

The critical security requirement that the minter must be authenticated as a super_user is combined with other concepts in a single sentence. Please present this security requirement in separate, distinct sentences to improve readability and scannability.

For example:

Pass `role` as an inline role-shaped object (the same `permission` structure used by [`add_role`](../users-and-roles/overview.md), including the `operations` allowlist). Unlike the username/password flow above, this shape carries no credentials of its own. The **minter** must be authenticated as a `super_user`. No `password` may be included in the body:
References
  1. Ensure critical security warnings, such as unauthorized access risks or fallback behaviors, are presented in separate, distinct sentences rather than being combined with other concepts or buried behind semicolons, so that readers scanning the documentation can easily find them.

1. The table is exported in a schema definition with [`@export`](../database/schema.md#export).
2. REST is enabled for the application — normally `rest: true` in `config.yaml` (see [Configuration](#configuration)); a component directory with **no configuration file at all** gets it from Harper's built-in default instead, as described below.
1. The table is exported in a schema definition with [`@export`](../database/schema.md#export). This is always required.
2. REST is enabled for the application. `rest: true` must be present in `config.yaml` **whenever a configuration file exists** (see [Configuration](#configuration)); a component directory with no configuration file at all inherits `rest` from Harper's built-in default instead, as described below.

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.

medium

The fallback behavior of inheriting rest from Harper's built-in default is buried behind a semicolon. Please present this fallback behavior in a separate, distinct sentence to make it more prominent and easier to scan.

For example:

2. REST is enabled for the application. `rest: true` must be present in `config.yaml` **whenever a configuration file exists** (see [Configuration](#configuration)). A component directory with no configuration file at all inherits `rest` from Harper's built-in default instead, as described below.
References
  1. Ensure critical security warnings, such as unauthorized access risks or fallback behaviors, are presented in separate, distinct sentences rather than being combined with other concepts or buried behind semicolons, so that readers scanning the documentation can easily find them.

@github-actions
github-actions Bot temporarily deployed to pr-662 September 2, 2026 14:32 Inactive
@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-662

This preview will update automatically when you push new commits.

@Ethan-Arrowood
Ethan-Arrowood merged commit c832245 into main Sep 2, 2026
11 checks passed
@Ethan-Arrowood
Ethan-Arrowood deleted the docs/fixes-from-skills-81 branch September 2, 2026 18:47
@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.

2 participants