Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .claude/skills/taos-development-skill/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Comment on lines +318 to +320

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Align the command claim with the actual CI gate.

The supplied repository contract runs npm run build, which executes tsc -b && vite build; tsconfig.json makes this non-emitting, but the executor is not shown running tsc --noEmit directly. Document the actual command or say “the typecheck/build gate” to avoid contributors validating a different check than CI.

Suggested wording
--  `tsc --noEmit` for exactly that reason (pitfall 21).
+-  the frontend typecheck/build gate for exactly that reason (pitfall 21).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- 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).
- 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
the frontend typecheck/build gate for exactly that reason (pitfall 21).
🧰 Tools
🪛 SkillSpector (2.3.11)

[warning] 415: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.

Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.

(Rogue Agent (RA2))


[error] 379: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).

Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.

(Tool Misuse (TM1))

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/taos-development-skill/SKILL.md around lines 286 - 288,
Update the guidance around the “Typecheck or run the thing” statement to
reference the repository’s actual CI command, npm run build, or use the neutral
phrase “typecheck/build gate.” Remove the claim that the executor directly runs
tsc --noEmit, while preserving the instruction to validate frontend changes
before opening a PR.

- 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
Expand Down
72 changes: 72 additions & 0 deletions docs/contributor-pitfalls.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Loading