mirror of
https://github.com/aphrody-labs/opentui.git
synced 2026-10-13 21:08:53 +00:00
skills: keep the opentui skill installable (#1579)
Fixes #1578. npx skills add anomalyco/opentui --skill opentui found the opentui skill only through the skills CLI's fallback search, and the CLI skips that search when a conventional location such as .opencode/skills/ holds a skill. The harden skill added in #1479 sat there, so the command reported "No matching skills found for: opentui". Move harden out of the repository and make validate:docs:skill fail when git tracks any SKILL.md other than the public skill.
This commit is contained in:
Simon Klee authored and GitHub committed
1 parent
5494865060
commit
5f6f286c3b
5 files changed
+15
-241
No files matched your search
@@ -1,92 +0,0 @@
|
||||
---
|
||||
name: harden
|
||||
description: >-
|
||||
Hardening pass for OpenTUI changes. Trace every changed code path. List every Zig and TypeScript
|
||||
branch arm that no test reaches, with the repo's branch-coverage tools. Find defects that survive
|
||||
an attempt to refute them. Fix them, simplify the code, and replace example tests with table,
|
||||
model, differential, and reflection tests. Keep source and test line counts flat or negative.
|
||||
Use for "harden", "find bugs in this change", "are all branches tested", "review this branch for
|
||||
defects", "improve test coverage without adding tests", "zero bug pass", or before you merge a
|
||||
large or risky branch in this repository.
|
||||
---
|
||||
|
||||
# Harden a change
|
||||
|
||||
A green test suite does not prove that a change is correct. Defects hide where example tests do not
|
||||
look: cross-layer seams, state-machine interleavings, hard limits, and unit boundaries.
|
||||
|
||||
Read `AGENTS.md` first. Load the `tigerstyle`, `code-simplification`, and `adversarial-code-review`
|
||||
skills. This skill applies their rules.
|
||||
|
||||
## Setup
|
||||
|
||||
- The default scope is `git diff main...HEAD`. The user can narrow it.
|
||||
- Edit in a new worktree (`git worktree add -b harden/<topic> ../ot-harden-<topic> HEAD`). Do not
|
||||
use `git stash`. All worktrees share the stash, and it holds the user's work.
|
||||
- Before you edit, run the affected suites and record the pass and fail counts. Then you know which
|
||||
later failures are new. The commands are in `references/coverage-tools.md`.
|
||||
- Run checks in the foreground. Do not end a turn while a background job runs.
|
||||
|
||||
## Step 1: measure what no test reaches
|
||||
|
||||
Run the branch-coverage tools on the files in scope:
|
||||
|
||||
```sh
|
||||
cd packages/native && bun run test:branch-cov # Zig files changed in main...HEAD
|
||||
cd packages/core && bun run test:branch-cov # TypeScript files changed in main...HEAD
|
||||
```
|
||||
|
||||
Each tool prints `UNCOVERED file:line:col` for each branch arm, error return, switch case, or
|
||||
function that no test ran. Options and limits are in `references/coverage-tools.md`.
|
||||
|
||||
## Step 2: trace and refute
|
||||
|
||||
For each changed function, list its branch arms: the uncovered arms from step 1 and the covered arms
|
||||
that you read. Compare each arm with the contract: `opentui.h`, the docs, and the behavior on
|
||||
`main`. Before you trace, read `references/fault-lines.md`.
|
||||
|
||||
A candidate is a defect only after you try to refute it and fail. Write down the attempt: is it the
|
||||
same on `main`, can the public API reach it, and does the caller already reject the input? Then
|
||||
reproduce the defect with a temporary probe outside the repo.
|
||||
|
||||
Report each defect with its location, trigger, expected and actual behavior, severity, evidence,
|
||||
and regression test. Record defects that also exist on `main`, but do not fix them unless they block
|
||||
a branch fix. If the scope is not clear, ask the user.
|
||||
|
||||
## Step 3: fix and simplify under a line budget
|
||||
|
||||
For each defect:
|
||||
|
||||
1. Write the regression test, and make sure that it fails.
|
||||
2. Fix the defect, and make sure that the test passes.
|
||||
3. Commit the test and the fix together (`core: ...` or `native: ...`).
|
||||
|
||||
Then do one of these for each arm that no test reaches, in order of preference:
|
||||
|
||||
1. **Delete it.** Change the code so that the state or branch does not exist.
|
||||
2. **Assert it.** If an invariant makes the arm unreachable, write an `assert`.
|
||||
3. **Cover it.** Extend a test of a shape in `references/test-shapes.md`. Do not add a scenario
|
||||
test.
|
||||
|
||||
For each unit of work, the net change in source lines must be zero or negative, and the net change
|
||||
in test lines must be negative. Give a one-line reason for each source addition in its commit
|
||||
message. After you edit, run step 1 again to make sure that the arms are gone from the report.
|
||||
|
||||
## Step 4: independent review
|
||||
|
||||
Give the diff to a session that did not write it. The reviewer must:
|
||||
|
||||
- Make sure that each regression test fails at the parent of its fix commit.
|
||||
- Break the code under each new or consolidated test in two or three plausible ways, and make sure
|
||||
that a test fails each time.
|
||||
- List each assertion that a consolidation removed, and each `main` API whose behavior changed.
|
||||
|
||||
"Merge after fixes" is the normal verdict.
|
||||
|
||||
## Large changes
|
||||
|
||||
Split a large branch into units by subsystem, with explicit file ownership. Do steps 1 and 2 for all
|
||||
units in parallel, without edits. Then do steps 3 and 4 for each unit in its own worktree, and merge
|
||||
each unit into an integration branch after review. The orchestrator owns one tracker file. Each
|
||||
agent writes only its own unit file: scope, branch inventory, defects, planned commits, and the line
|
||||
change of each commit.
|
||||
@@ -1,62 +0,0 @@
|
||||
# Coverage tools and baseline commands
|
||||
|
||||
## Native: `packages/native/scripts/branch-cov.ts`
|
||||
|
||||
```sh
|
||||
cd packages/native
|
||||
bun run test:branch-cov # Zig files changed in main...HEAD, full suite
|
||||
bun scripts/branch-cov.ts --diff <range> # files changed in another range
|
||||
bun scripts/branch-cov.ts --files <file>... # explicit files
|
||||
bun scripts/branch-cov.ts --all # every file under src/ except vendor, tests, bench
|
||||
bun scripts/branch-cov.ts ... --filter <name> # only tests whose name contains <name>
|
||||
bun scripts/branch-cov.ts ... --json <path> # also write the per-arm table
|
||||
```
|
||||
|
||||
`zig build test-cov` builds the tests with `fuzz = true`, so LLVM gives each basic block a counter.
|
||||
`src/cov_runner.zig` runs the tests and writes the counters. The script maps each counter to a
|
||||
source location with `objdump` and `llvm-symbolizer`, which must be on `PATH`. A filtered run takes
|
||||
about 15 s, and the full suite takes about 60 s.
|
||||
|
||||
Limits:
|
||||
|
||||
- All instantiations of a generic function share one site.
|
||||
- The report excludes vendored C/C++.
|
||||
- Zig does not compile unreferenced functions, so the report does not show them. Use `grep` to make
|
||||
sure that a function still has callers.
|
||||
|
||||
## Core TypeScript: `packages/core/scripts/branch-cov.ts`
|
||||
|
||||
```sh
|
||||
cd packages/core
|
||||
bun run test:branch-cov # TS files changed in main...HEAD, whole suite (~1 min)
|
||||
bun scripts/branch-cov.ts --diff <range> # files changed in another range
|
||||
bun scripts/branch-cov.ts --files <file>... --tests <test file>...
|
||||
bun scripts/branch-cov.ts ... --json <path> # also write the per-arm table
|
||||
```
|
||||
|
||||
`scripts/branch-cov-preload.ts` collects JavaScriptCore block coverage through `node:inspector`.
|
||||
`scripts/branch-cov-map.ts` maps the ranges in the transpiled JavaScript back to TypeScript lines.
|
||||
Line numbers are 98 to 100 % accurate. Columns are approximate.
|
||||
|
||||
Limits:
|
||||
|
||||
- The report excludes `import` lines.
|
||||
- For a file that no test imports, the tool prints "not loaded by any test".
|
||||
- The tool runs under Bun only, not Node.
|
||||
|
||||
## Baseline suites
|
||||
|
||||
Run each suite before and after you edit.
|
||||
|
||||
| Suite | Directory | Command |
|
||||
| --------------------------------- | ----------------------------------- | ------------------------------------------------------------------- |
|
||||
| Native | `packages/core` | `bun run test:native` |
|
||||
| Native lifetime / ABI | `packages/core` / `packages/native` | `bun run test:native:lifetime` / `zig build test-abi --summary all` |
|
||||
| ABI against header, all targets | `packages/core` | `bun run check:abi --all-targets` |
|
||||
| Core (Bun / Node) | `packages/core` | `bun run test` / `bun run test:js:node` |
|
||||
| Core dist | `packages/core` | `bun run test:dist` (not in parallel with the solid tests) |
|
||||
| ssh, react, solid, qrcode, keymap | each package | `bun run test` |
|
||||
| Format / lint | root | `bun run fmt:check` / `bun run lint` |
|
||||
|
||||
The build fails if an exported `ot_*` function does not match `opentui.h`. For each ABI change,
|
||||
update the header and run `bun run generate:abi` in `packages/core`.
|
||||
@@ -1,49 +0,0 @@
|
||||
# Where defects hide
|
||||
|
||||
When you trace a changed function, examine each group.
|
||||
|
||||
## Limits and capacities
|
||||
|
||||
Test each fixed limit just below, at, and just above the limit. When a capacity is full, the
|
||||
operation must fail clearly. It must not drop data silently or block later operations.
|
||||
|
||||
## Units of text measurement
|
||||
|
||||
Bytes, UTF-16 code units, code points, graphemes, and display cells are different numbers.
|
||||
Arithmetic that mixes two of them is a defect. Test text APIs with ASCII, wide characters,
|
||||
zero-width combining marks, multi-code-point emoji, tabs, `\r\n`, control characters, and a grapheme
|
||||
cluster longer than the limit.
|
||||
|
||||
## Lifecycle
|
||||
|
||||
Examine calls after destroy, close, or suspend, and calls during teardown. A failed native call must
|
||||
not leave JS state that refers to a destroyed object. Native code must not keep a callback after its
|
||||
owner stops. A normal shutdown must not report an error.
|
||||
|
||||
## Busy and retry states
|
||||
|
||||
When a queue is full or output is busy, the caller must get a result that it can retry, not a
|
||||
permanent failure. A rejected item must not block the items after it. The code must not lose an
|
||||
event that arrives during a busy state. Compare each behavior with `main`.
|
||||
|
||||
## Caller inputs
|
||||
|
||||
Give setters and callbacks `null`, `undefined`, `NaN`, `Infinity`, `-0`, negative values, and very
|
||||
large values. Give text APIs control characters. TypeScript must not keep a value that native code
|
||||
rejects.
|
||||
|
||||
## Cross-layer contracts
|
||||
|
||||
The header, the docs, and the code must agree. ABI checks must cover record layouts, not only
|
||||
function prototypes. A TypeScript shortcut must not skip a native call that native code needs. An
|
||||
error in one layer must not drop the rest of a batch in another layer.
|
||||
|
||||
## Caches and fast paths
|
||||
|
||||
Work must be proportional to what the call needs, not to the whole structure. Each dirty flag must
|
||||
schedule the work that it promises. Assert work counts, not wall-clock time.
|
||||
|
||||
## Fixes
|
||||
|
||||
A fix can move a failure instead of removing it, change the timing of an API, or remove the only
|
||||
check of a property. Review each fix as carefully as the code that it fixes.
|
||||
@@ -1,38 +0,0 @@
|
||||
# Test shapes
|
||||
|
||||
Use these shapes instead of a new test with one hand-written sequence.
|
||||
|
||||
## Seeded model test
|
||||
|
||||
For a stateful structure, apply the same random operations from a fixed seed to the real structure
|
||||
and to a small reference model. Compare the observable state after each step. Limit the number of
|
||||
operations, and print the seed and step on failure. Write the model independently of the
|
||||
implementation. Include stale and invalid inputs in the operations.
|
||||
|
||||
## Differential test
|
||||
|
||||
For a cache, batch, reuse pool, incremental update, or recording, assert that the fast path and a
|
||||
fresh reference path give the same result on random inputs.
|
||||
|
||||
## Reflection sweep
|
||||
|
||||
To cover a whole API surface, enumerate it from the code, not from a hand-written list. Then the
|
||||
test covers new members automatically. Keep an exclusion list with a reason for each entry.
|
||||
|
||||
## Table test
|
||||
|
||||
If tests repeat the same calls with different literals, make the literals rows. Name the bug that a
|
||||
row pins. When you convert tests to a table, compare the old assertions one by one, because
|
||||
conversions lose rows.
|
||||
|
||||
## Fixture checks
|
||||
|
||||
Put the checks that every test needs into the shared fixture, such as leaks, live handles, pending
|
||||
timers, and listeners. Then each existing test does these checks.
|
||||
|
||||
## Do not write
|
||||
|
||||
- A scenario test for one happy path. Make it a table row.
|
||||
- A wall-clock timing assertion. Count work instead.
|
||||
- A test that repeats the implementation.
|
||||
- A regression test that you did not see fail on the unfixed code.
|
||||
@@ -1,5 +1,6 @@
|
||||
#!/usr/bin/env bun
|
||||
|
||||
import { execFileSync } from "node:child_process"
|
||||
import { readFile } from "node:fs/promises"
|
||||
import { join } from "node:path"
|
||||
|
||||
@@ -39,6 +40,7 @@ async function main() {
|
||||
|
||||
const skillContent = await readFile(join(REPO_ROOT, SKILL_SOURCE_PATH), "utf8")
|
||||
violations.push(...validateSkillIndex(index, skillContent))
|
||||
violations.push(...validateOnlySkill())
|
||||
|
||||
if (violations.length > 0) {
|
||||
console.error("Skill doc validation failed:\n")
|
||||
@@ -196,6 +198,19 @@ function validateSkillIndex(index: DocsIndex, content: string): Violation[] {
|
||||
return violations
|
||||
}
|
||||
|
||||
function validateOnlySkill(): Violation[] {
|
||||
const paths = execFileSync("git", ["ls-files", "--", ":(glob)**/SKILL.md"], { cwd: REPO_ROOT, encoding: "utf8" })
|
||||
return paths
|
||||
.split("\n")
|
||||
.filter((path) => path && path !== SKILL_SOURCE_PATH)
|
||||
.map((sourcePath) => ({
|
||||
rule: "only-skill",
|
||||
sourcePath,
|
||||
lineNumber: 1,
|
||||
message: `${SKILL_SOURCE_PATH} must be the only tracked SKILL.md; npx skills add anomalyco/opentui --skill opentui finds it only when no other skill is in a conventional location such as .opencode/skills/`,
|
||||
}))
|
||||
}
|
||||
|
||||
function normalizeDocUrl(url: string): "/docs" | `/docs/${string}` {
|
||||
return (url.endsWith("/") ? url.slice(0, -1) : url) as "/docs" | `/docs/${string}`
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user