From b9697f239fda2eb2dbd6767b0d1c6b4d6b6dc448 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Tue, 21 Jul 2026 16:01:14 +0000 Subject: [PATCH 1/4] docs(skill+pitfalls): three defect classes from the Library P1 review cycle Pitfalls: tolerances do not test invariants (20), shell snippets in template literals must escape the interpolation sigil (21), conflict resolution reverts fixes when done mechanically (22). All three are drawn from real defects in this cycle, including a data-loss bug that a tolerance-based assertion kept green and 225 syntax errors from one unescaped construct. Skill: a verify-before-you-claim section covering tolerance assertions, mocks built from the caller's assumptions rather than the service contract, and compiling before opening a PR. --- .../skills/taos-development-skill/SKILL.md | 16 ++++++++++ docs/contributor-pitfalls.md | 31 +++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/.claude/skills/taos-development-skill/SKILL.md b/.claude/skills/taos-development-skill/SKILL.md index f4a96feef..0a5250747 100644 --- a/.claude/skills/taos-development-skill/SKILL.md +++ b/.claude/skills/taos-development-skill/SKILL.md @@ -274,6 +274,22 @@ 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). +### 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). +- A test that mocks an external service must build its fixture from that + service's documented response, never from the shape the calling code expects. + A green test over a fictional contract is worse than no test: it certifies the + bug. This happened three times in one PR cycle. +- 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..92d51d886 100644 --- a/docs/contributor-pitfalls.md +++ b/docs/contributor-pitfalls.md @@ -154,3 +154,34 @@ 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. From 52db125e933ee3dbb11747585d22acf6915759f3 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Tue, 21 Jul 2026 16:07:14 +0000 Subject: [PATCH 2/4] docs(skill+pitfalls): external-contract mocks need a captured source Strengthens the mock guidance from a general principle into an enforceable rule. Scopes the danger to mocks of services we do not control, requires a captured real response over a hand-composed fixture, asks for one feature detecting integration test per contract, and rejects a follow-up issue as sufficient mitigation since it detaches the caveat from the code. --- .../skills/taos-development-skill/SKILL.md | 13 +++++++--- docs/contributor-pitfalls.md | 25 +++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/.claude/skills/taos-development-skill/SKILL.md b/.claude/skills/taos-development-skill/SKILL.md index 0a5250747..18c3c8229 100644 --- a/.claude/skills/taos-development-skill/SKILL.md +++ b/.claude/skills/taos-development-skill/SKILL.md @@ -279,10 +279,15 @@ into git forensics (this happened with #1927/#1924 - both were legitimate - 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). -- A test that mocks an external service must build its fixture from that - service's documented response, never from the shape the calling code expects. - A green test over a fictional contract is worse than no test: it certifies the - bug. This happened three times in one PR cycle. +- Mocking internals or injecting unreproducible errors is fine. Mocking an + EXTERNAL service contract is where tests lie. Capture a real response and + commit it as the fixture rather than composing one from what your code + expects, keep one real integration test per external contract (feature + detecting so it skips when the service is unreachable), and if you can do + neither, mark the mock provisional in code with its source and verification + date. A follow-up issue does not count: it separates the caveat from the code. + A green test over a fictional contract is worse than no test, it certifies the + bug. This happened three times in one PR cycle (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). diff --git a/docs/contributor-pitfalls.md b/docs/contributor-pitfalls.md index 92d51d886..0c409e5fa 100644 --- a/docs/contributor-pitfalls.md +++ b/docs/contributor-pitfalls.md @@ -185,3 +185,28 @@ 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. + +Requirements for that class, strongest first: + +1. **Capture, do not compose.** Call the real service once and commit its + response as the fixture. A recorded response cannot encode a wrong belief. +2. **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. +3. **If neither is possible, 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. From 1713f7594d5cf08639b7ed8520d6aba8130623b9 Mon Sep 17 00:00:00 2001 From: jaylfc Date: Tue, 21 Jul 2026 16:08:06 +0000 Subject: [PATCH 3/4] docs(skill+pitfalls): split contract mocks by whether the owner is reachable Sibling services are not third parties. taOSmd's maintainer is on the A2A bus, so a contract question costs one message. Every mock failure in the 2062 cycle was a guess at something that was free to ask, and asking produced documented envelopes, a self-corrected stats key list, and a capabilities endpoint. Guessing is only justified when there is genuinely nobody to ask. --- .../skills/taos-development-skill/SKILL.md | 18 ++++++------ docs/contributor-pitfalls.md | 28 +++++++++++++++---- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/.claude/skills/taos-development-skill/SKILL.md b/.claude/skills/taos-development-skill/SKILL.md index 18c3c8229..60099c324 100644 --- a/.claude/skills/taos-development-skill/SKILL.md +++ b/.claude/skills/taos-development-skill/SKILL.md @@ -280,14 +280,16 @@ into git forensics (this happened with #1927/#1924 - both were legitimate 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. Capture a real response and - commit it as the fixture rather than composing one from what your code - expects, keep one real integration test per external contract (feature - detecting so it skips when the service is unreachable), and if you can do - neither, mark the mock provisional in code with its source and verification - date. A follow-up issue does not count: it separates the caveat from the code. - A green test over a fictional contract is worse than no test, it certifies the - bug. This happened three times in one PR cycle (pitfall 23). + 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). diff --git a/docs/contributor-pitfalls.md b/docs/contributor-pitfalls.md index 0c409e5fa..b65b265cc 100644 --- a/docs/contributor-pitfalls.md +++ b/docs/contributor-pitfalls.md @@ -195,15 +195,31 @@ 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. -Requirements for that class, strongest first: - -1. **Capture, do not compose.** Call the real service once and commit its - response as the fixture. A recorded response cannot encode a wrong belief. -2. **Keep one real integration test per external contract.** Have it feature +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. -3. **If neither is possible, mark the mock provisional IN CODE** with the +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. From 52b8390e32efca2faa976f2fd4400800e9895a6b Mon Sep 17 00:00:00 2001 From: jaylfc Date: Tue, 21 Jul 2026 16:33:07 +0000 Subject: [PATCH 4/4] docs(skill): how an external contributor reaches another team's agent Adds the routing rule that makes pitfall 23 actionable for contributors who are not on the A2A bus: a contract question goes to the commons repo (or taosmd directly, which is public), the lead relays to the owning agent, and the contributor escalates on the PR if it stalls. Marked explicitly as temporary scaffolding that retires when contributors can hold a taOS identity. --- .../skills/taos-development-skill/SKILL.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/.claude/skills/taos-development-skill/SKILL.md b/.claude/skills/taos-development-skill/SKILL.md index 60099c324..75f7d1dc2 100644 --- a/.claude/skills/taos-development-skill/SKILL.md +++ b/.claude/skills/taos-development-skill/SKILL.md @@ -274,6 +274,31 @@ 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.