diff --git a/.claude/skills/taos-development-skill/SKILL.md b/.claude/skills/taos-development-skill/SKILL.md index f4a96feef..75f7d1dc2 100644 --- a/.claude/skills/taos-development-skill/SKILL.md +++ b/.claude/skills/taos-development-skill/SKILL.md @@ -274,6 +274,54 @@ A close without a successor link reads as lost work and forces the maintainer into git forensics (this happened with #1927/#1924 - both were legitimate "landed via" closures that looked like data loss for hours). +### Asking another team's agent a question + +taOS depends on sibling services with their own maintainer agents (taOSmd, the +website). Their contracts are theirs to state, so ASK rather than inferring from +their source (pitfall 23). How to reach them depends on where you sit: + +- **On the A2A bus** (internal agents): post on the relevant channel, name the + agent, and expect a reply inside the hour. +- **Outside the bus** (external contributors): open an issue on + `jaylfc/taos-agent-commons`, the private invite-only coordination repo. Label + it `contract-question` and name the service. @taOS-dev sweeps it hourly and + relays to the owning agent on the bus, then carries the answer back. +- `jaylfc/taosmd` is public with issues enabled, so a taosmd contract question + can also go straight there. +- `jaylfc/taos-website` is private, so commons or the relay is the only route. + +If a question sits unanswered for more than about two hours, escalate by also +raising it on the PR. The relay is a person-shaped hop and can stall; silence +should never be mistaken for progress. + +This arrangement is TEMPORARY scaffolding. It retires when an external +contributor can hold a taOS identity and reach the bus directly, which is the +same capability as agent sharing. Do not build tooling that assumes it is +permanent. + +### Verify before you claim, and compile before you PR + +- An assertion with a tolerance (`abs(a - b) <= n`) does not test an invariant. + It cannot tell correct behaviour from total failure. Assert equality and + assert the resulting state (pitfall 20). +- Mocking internals or injecting unreproducible errors is fine. Mocking an + external service CONTRACT is where tests lie. For sibling services (taOSmd, + the website) the contract has a reachable owner on the A2A bus: ASK them + rather than inferring from their source. Every mock failure in the #2062 + cycle was a guess at something one message would have answered. External + contributors ask in the PR and the lead relays. For genuine third parties, + capture a real response and commit it rather than composing a fixture from + what your code expects. Then keep one feature detecting integration test per + contract, or mark the mock provisional in code with its source and date. A + follow-up issue does not count: it separates the caveat from the code. A + green test over a fictional contract certifies the bug (pitfall 23). +- Typecheck or run the thing before opening the PR. A frontend change that does + not compile wastes a full review round, and the executor now gates on + `tsc --noEmit` for exactly that reason (pitfall 21). +- When the base branch has moved under you, rebase and read what changed before + resolving conflicts. Taking the wrong side of a hunk silently reverts fixes + that were just merged (pitfall 22). + ### Scope honesty per slice - The PR body states exactly what it ships versus what its issue scopes. Any diff --git a/docs/contributor-pitfalls.md b/docs/contributor-pitfalls.md index 56bc1be74..b65b265cc 100644 --- a/docs/contributor-pitfalls.md +++ b/docs/contributor-pitfalls.md @@ -154,3 +154,75 @@ say so in the PR body and file the follow-up issue in the same push. Never let a partial slice close the parent issue. Silent shortfalls read as done, get caught in review anyway, and cost a full extra round (#2042: community chat and peer access absent with no deferral note). + +**20. A tolerance is not a test of an invariant.** +An assertion like `assert abs(after - before) <= 2` cannot distinguish correct +behaviour from catastrophic failure. In PR #2062 that exact assertion ran green +while reprocess destroyed the user's original uploaded file: the observed values +were `before=2, after=0`, which the tolerance accepted, and the same tolerance +would equally have accepted a doubling to 4. If the property is "the count does +not change", assert equality and assert the resulting status, so the test fails +loudly on both loss and duplication. Reserve tolerances for genuinely +approximate quantities such as timings, and even then bound them tightly. + +**21. Shell snippets inside template literals must escape `${`.** +A bash or PowerShell snippet stored in a JavaScript template literal collides +with the language's own interpolation: `${VAR:-default}` is parsed as JS, not +shell. In PR #2077 this produced 225 TypeScript syntax errors from a single +cause, in one of four otherwise-clean files, and read like incoherent output +rather than one mechanical mistake. Escape as `\${`, or keep snippets in plain +non-template strings or separate asset files. The class generalises to any +language sharing `${...}` with the shell, and it is invisible to anything that +does not actually compile the file, which is why the frontend typecheck gate +exists before a PR is opened. + +**22. Conflict resolution is a decision, not a mechanical act.** +Taking the wrong side of a hunk silently reverts fixes that were just made. When +a base branch has moved substantially under a long-lived branch, read what +changed underneath before resolving: the four defects fixed in Library P1 +(nested response envelope, the guard preventing reprocess from deleting the +source upload, the compare-and-swap status transition, exact artifact-count +assertions) are all reintroducible by a plausible-looking resolution. Rebase +rather than merging the base in, so the diff stays reviewable and each conflict +is seen individually. + +**23. Mocks of EXTERNAL service contracts need a real source, not a guess.** +Mocking our own internals or injecting errors you cannot produce on demand (a +500, a timeout, an ImportError) is fine and unavoidable. The dangerous class is +narrow: a mock of a service we do not control, whose fixture was hand-written +from what the calling code expects. That fixture encodes a BELIEF about someone +else's API, and when the belief is wrong the test proves nothing while going +green. This happened three times in one PR cycle (#2062): an invented `dbPath` +request shape, an un-nested poll response, and a tolerance assertion over both. + +First, split the class by whether the contract has a reachable OWNER. + +**Sibling services (taOSmd, taOS website): ASK. Do not guess.** These are not +third parties. Their maintainer is on the A2A bus, and asking costs one message. +Every failure in the #2062 cycle came from inferring a contract that was free to +obtain: the builder guessed a request shape, and the reviewer verified against +source rather than asking the owner. When we finally asked, we got the envelope +documented, a wrong stats key list corrected by its own author, and a +`/version` capabilities endpoint built to make the contract machine-checkable. +Reverse-engineering a sibling's API from its source is a smell, not diligence: +source tells you what it does today, the owner tells you what it guarantees. +Contributors without bus access (external collaborators) ask in the PR, and the +lead relays. + +**Genuinely third-party (GitHub, OpenRouter, Reddit): capture, do not compose.** +There is no one to ask, so call the real service once and commit its response as +the fixture. A recorded response cannot encode a wrong belief. + +For both, then: + +1. **Keep one real integration test per external contract.** Have it feature + detect and skip when the service is unreachable, so CI stays green offline + but drift surfaces the moment anyone runs it against a live instance. For + taosmd, `GET /version` returns a capabilities list for exactly this. +2. **If that is impossible, mark the mock provisional IN CODE** with the + contract source and the date it was verified. A follow-up issue is not + sufficient: it detaches the caveat from the code and, in a tracker with + hundreds of open items, functions as indefinite deferral. + +Reviewer's job: ask where the fixture came from. A green test over a fictional +contract is worse than no test, because it certifies the bug.