-
-
Notifications
You must be signed in to change notification settings - Fork 34
Merge dev into master: v1.0.0-beta.43 #2087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
18e9928
7f37f07
37fb4b3
6ee9f63
abf67a4
4498855
056350a
3878ccb
61a0769
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,10 @@ session + CSRF, or the agent-token allowlist in `auth_middleware.py`) and a | |
| test asserting an unauthenticated or cross-principal caller gets 401/403. | ||
| Example: PR #2036 added `GET /api/secrets/agent/{name}/github` with no guard, | ||
| leaking installation IDs and repo names to any caller. | ||
| For project-scoped routes, copy the house guard verbatim: | ||
| `if not user.is_admin and user.user_id != p["user_id"]` followed by a masked | ||
| 404 (see routes/projects.py). A bare 403 without the admin bypass both locks | ||
| out admins and leaks resource existence to other users (#2042). | ||
|
|
||
| **2. Bind both directions on authenticated channels.** | ||
| When a message or envelope arrives over an authenticated channel, verify BOTH | ||
|
|
@@ -37,6 +41,16 @@ and strips it. Tokens we only VERIFY are stored hashed; tokens we must PRESENT | |
| outbound are the only ones stored recoverable. Example: PR #2009 moved the | ||
| GitHub App RSA key out of plaintext config.yaml. | ||
|
|
||
| **16. Never commit runtime state or key material.** | ||
| Anything a store or subsystem writes under `data/` at runtime is state, not | ||
| source. A PR that introduces a component writing under `data/` must add that | ||
| directory to `.gitignore` in the SAME PR, and `git status` must be checked for | ||
| runtime artifacts before every commit. Any private key that reaches a pushed | ||
| commit is burned: regenerate it, and keep it out of the target branch's history | ||
| (squash merge, or rewrite the branch). Example: `data/hub/identity.json` with | ||
| live signing/encryption keys entered one branch's history and was then | ||
| re-committed by a second PR (#2043 history, #2042). | ||
|
|
||
| ## Correctness | ||
|
|
||
| **5. Never take element `[0]` of a collection that can hold more than one.** | ||
|
|
@@ -74,6 +88,12 @@ share flow), state it in the PR body and file a follow-up issue. Example: PR | |
| permissions. Wire the real value or do not add the parameter yet. Example: | ||
| PR #2036 `handleSaveGrants`. | ||
|
|
||
| **17. A new view must be wired into every surface it has: desktop AND mobile.** | ||
| The desktop tab list and the mobile tab order are separate registries; updating | ||
| one and not the other ships a view that is unreachable on phones (#2042: | ||
| `TABS` updated, `mobileTabOrder` missed). Grep for every registry the sibling | ||
| views appear in and update all of them. | ||
|
|
||
| ## Store and schema | ||
|
|
||
| **11. SCHEMA is the frozen v1; new columns and their indexes go in MIGRATIONS.** | ||
|
|
@@ -87,6 +107,18 @@ Running a data migration twice must be a no-op (existence checks, INSERT OR | |
| IGNORE, migrated-from markers) and there must be a test proving the second run | ||
| changes nothing. Example done right: PR #2028 `test_migrate_idempotent`. | ||
|
|
||
| **18. Retrofitting a column onto an already-shipped store needs a guarded | ||
| ALTER, not just a MIGRATIONS entry.** | ||
| The migration runner baselines pre-existing databases at the latest version | ||
| WITHOUT executing the migrations (FOOTGUN #2 in `db_migrations.py`'s own | ||
| docstring), so a plain `MIGRATIONS = [(1, "ALTER TABLE ...")]` on a store that | ||
| already shipped is a silent no-op on every upgraded install: fresh DBs work, | ||
| upgraded DBs lose the feature at runtime. Use the guarded `_post_init` pattern | ||
| (PRAGMA table_info, ALTER only when the column is absent - see | ||
| `knowledge_store._migration_v1_add_user_id`) and add an upgrade test that | ||
| builds the PRE-change schema first. Fresh-DB tests are structurally blind to | ||
| this class (#2043: `peer_fingerprint`). | ||
|
|
||
| ## Process | ||
|
|
||
| **13. A fix and its test belong in one PR.** | ||
|
|
@@ -110,3 +142,87 @@ Findings are gated on severity of content, not review state. Address each one | |
| (fix it or rebut it concretely in the thread); never merge past an open | ||
| finding, and never let a "pass" verdict from a stale commit stand in for the | ||
| current head. | ||
| A finding is closed only when it is ANSWERED IN-THREAD: reply to each numbered | ||
| item with the commit that addresses it or a concrete rebuttal. Pushing code | ||
| without replies leaves the finding open - the reviewer re-verifies blind and | ||
| the verdict stays HOLD (#2043 round two). | ||
|
|
||
| **19. State scope deltas against the issue explicitly.** | ||
| If a PR ships less than its issue scopes - a subfeature dropped, owner-only | ||
| routes where the issue says peer-serving, a "live" view that fetches once - | ||
| 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. | ||
|
Comment on lines
+212
to
+214
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Require sanitized fixtures for captured external responses. Both rules should prohibit raw response commits and require minimal fixtures from safe test accounts with secrets, tokens, cookies, PII, and restricted fields removed.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
|
|
||
| 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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ Any of the following will work. More RAM means bigger, more capable models. | |
| |--------|-----|-------| | ||
| | **Orange Pi 5 Plus** (recommended) | 16 GB | RK3588 chip with 6 TOPS NPU for fast inference | | ||
| | **Orange Pi 5** | 8–16 GB | Same NPU, slightly fewer I/O ports | | ||
| | **Raspberry Pi 5** | 8 GB minimum | CPU-only inference unless you add an accelerator HAT | | ||
| | **Raspberry Pi 5** | 8 GB minimum | CPU-only inference unless you add an accelerator HAT; with the **AI HAT+2 (Hailo-10H, 40 TOPS)** you get NPU-accelerated LLM inference | | ||
| | **Any x86/x64 PC or laptop** | 4 GB+ | Budget PC, old laptop, NUC, etc. GPU optional | | ||
| | **NVIDIA GPU system** | 4 GB+ VRAM | GTX 1050 Ti and up; CUDA acceleration | | ||
| | **AMD GPU system** | 8 GB+ VRAM | RX 6600 and up; ROCm acceleration | | ||
|
|
@@ -31,6 +31,8 @@ The platform itself uses roughly 345 MB of RAM when idle, so it runs comfortably | |
|
|
||
| **Not sure which to buy?** The Orange Pi 5 Plus with 16 GB is the recommended starting point. It has a built-in NPU (neural processing unit) that runs models significantly faster than the CPU alone, and 16 GB gives you room to run multiple agents at once. | ||
|
|
||
| > **AI HAT+2 (Hailo-10H) and the Pi 5 M.2 slot:** The AI HAT+2 NPU accelerator occupies the Raspberry Pi 5's only M.2 slot, the same slot normally used for an NVMe boot drive. You cannot use the HAT and an NVMe SSD at the same time. If you want fast storage alongside Hailo LLM acceleration, use a USB SSD instead. | ||
|
|
||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Use PCIe terminology consistently for AI HAT+2. The AI HAT+2 connects through Raspberry Pi 5’s PCIe port; the M.2 HAT+ provides the M.2 connector for NVMe and uses that same PCIe interface. Update both descriptions while preserving the valid recommendation to use USB storage alongside the accelerator. (raspberrypi.com)
📍 Affects 2 files
🤖 Prompt for AI AgentsSource: MCP tools |
||
| ### Software | ||
|
|
||
| - **OS:** Armbian or Debian-based Linux (Ubuntu works too). The installer handles everything else. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -315,6 +315,44 @@ async def test_revoke_nonexistent_returns_false(store): | |
| assert await store.revoke("000000") is False | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_revoke_expired_invite(store): | ||
| """An expired invite can still be revoked (cleanup from the pending list).""" | ||
| result = await store.mint( | ||
| project_id="prj-1", | ||
| scopes=[], | ||
| approval_mode="auto", | ||
| check_interval_secs=1800, | ||
| created_by="u", | ||
| ) | ||
| iid = result["record"]["invite_id"] | ||
| await store._db.execute( | ||
| "UPDATE project_invites SET expires_ts = 1 WHERE invite_id = ?", (iid,) | ||
| ) | ||
| await store._db.commit() | ||
| row = await store.get(iid) | ||
| assert row["status"] == "expired" | ||
| ok = await store.revoke(iid) | ||
| assert ok is True | ||
| row = await store.get(iid) | ||
| assert row["status"] == "revoked" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_revoke_redeemed_invite_returns_false(store): | ||
| result = await store.mint( | ||
| project_id="prj-1", | ||
| scopes=[], | ||
| approval_mode="auto", | ||
| check_interval_secs=1800, | ||
| created_by="u", | ||
| ) | ||
| iid = result["record"]["invite_id"] | ||
| pin = result["pin"] | ||
| await store.redeem(iid, pin) | ||
| assert await store.revoke(iid) is False | ||
|
Comment on lines
+342
to
+353
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Exercise the redeemed state, not only claimed.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # redeem | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
There was a problem hiding this comment.
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
Fix the defect-class count.
This says “seven” but lists eight classes: runtime state, mobile registries, migrations, scope honesty, tolerance assertions, shell snippets, conflict resolution, and external-agent coordination.
🤖 Prompt for AI Agents