HarperFast / HarperFast/skills
harper-best-practices teaches un-awaited MaybePromise bodies and the legacy instance-verb form (with the wrong argument order)
- Dominant language
- JavaScript
- Stars
- 3
- Forks
- 0
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 8
Description
Three content defects found while vendoring `harper-best-practices` into two first-party Harper apps and reviewing the rules against Harper core. All are in the published package (`@harperfast/skills@1.12.4`) and all teach an agent something that fails silently at runtime.
## 1. `MaybePromise` body used without `await` — and the two shipped artifacts disagree
`rules/handling-binary-data.md` gets it right:
```js
const record = await super.get(target);
```
`rules/using-blob-datatype.md:63` does not:
```js
static async get(target) {
const record = super.get(target); // <- no await
let blob = record.data; // undefined
blob.on('error', () => { ... }); // TypeError -> 500 on every blob read
```
The concatenated `harper-best-practices/AGENTS.md` (line ~581) carries the un-awaited version too, so the artifact that non-Claude tools read is the wrong one. Two rules in the same skill contradict each other on the same API.
This is the failure mode harper's own `resources/DESIGN.md` → Conventions calls out: a static override shadows the `transactional()` wrapper, so `data`/the record arrive unresolved, and because a promise has no own enumerable properties the mistake is silent rather than loud — `JSON.stringify(body) === '{}'` and every field reads `undefined`.
## 2. Same class, write path
`AGENTS.md` ~line 656 (`using-blob-datatype`):
```ts
static async post(target: RequestTargetOrId, record: any) {
if (record.data) { // record is a MaybePromise -> falsy branch
record.data = createBlob(...);
}
return super.post(target, record);
```
An unresolved `record` skips the `createBlob` branch entirely and the raw base64 string is stored, with no error. Worse than the read case, because it corrupts data instead of throwing.
## 3. `checking-authentication.md` teaches the legacy form, with the wrong argument order
`rules/checking-authentication.md:52`:
```js
export class SignIn extends Resource {
async post(_target, data) {
const context = this.getContext();
```
Two problems. It is an **instance** verb, which contradicts `custom-resources.md` and `extending-tables.md` in the same skill (both prescribe `static`), and the v5 docs (*"For new code, prefer static methods and omit the flag"*). And the `(target, data)` signature is only correct when `static loadAsInstance = false` is set — which this example does not set. In the default mode harper dispatches `post(data, query)`, so `_target` receives the body and `data` receives the query. Following this example verbatim produces an endpoint whose `data` is not the request body.
Related: #78 (no rule covers `loadAsInstance` or migrating off the instance-verb form). This issue is the flip side — a rule that actively teaches the form #78 says is legacy.
## Why it matters more now
These rules are being vendored into first-party Harper apps so agents load them by default. `checking-authentication.md` in particular is the rule an agent reaches for when writing an auth endpoint, and it is uncontradicted at the point of use.
## Suggested fix
1. Add the missing `await` in `using-blob-datatype.md` and regenerate `AGENTS.md`, then check whether the generator can drift from its sources — if the concatenation is generated, #1 suggests it either isn't regenerated in lockstep or the source was fixed in only one place.
2. Fix the write-path example the same way.
3. Convert `checking-authentication.md` to the static form, matching the rest of the skill.
4. Consider a lint over the rule corpus for `super.get(`/`super.post(` without `await` and for instance-verb declarations — all three of these are mechanically detectable.
Contributor guide
Assessment
This issue has not been assessed yet.