update roadmap 2 #67
update roadmap 2 #67
Conversation
flyingrobots
commented
Nov 18, 2025
flyingrobots
commented
- feat(adr): add ADR-0004 for hybrid privacy model
- docs(adr): enhance ADR-0004 with clarifications and alternatives considered
- feat(adr): add ADR-0005 for shiplog event stream
- docs(adr): enhance ADR-0005 with clarifications and alternatives
- docs ADR-0005 udpates
- Add ADR-0006, sweep ADR-0005
- docs ADR-0006 example schemas
- ADR 0006-0017
- ADR-0007 fix some details that were missing
- docs: more ADR refinement
- docs: ADR-0010 updates
- docs: Updates based on ADRs
- docs: expand proof + schema coverage
- docs: clarify REST scopes and dlq ops
- docs: document federated stream semantics
- docs: capture GH approval bridge + queue ops
- docs: define export envelope + samples
- docs: lock down federation discovery
- docs: define fold prewarm + cache policy
- chore: Cargo.lock update
- docs: normalize ADR metadata and roadmap
- docs: add structured task backlog
- docs: add ADRs for sessions and PoX
- docs: align spec and guide with sessions & PoX
- docs: add operations & observability chapter
Summary by CodeRabbit
WalkthroughThis diff introduces a comprehensive architectural redesign and expansion of the GATOS project. Key changes include renaming the message-bus component from "gatos-mind" to "gatos-message-plane," introducing a new GraphQL API crate, adding local enforcement mechanisms (watcher daemon and Git hooks), creating multiple decision records (ADRs-0004 through ADR-0016), reorganizing and expanding documentation, and defining numerous JSON schemas for new API surfaces. The workspace members are updated accordingly, and examples are provided for new features across API, export, federation, message-plane, privacy, state, and watch domains. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + pub struct GraphQlRequest { | ||
| + pub query: String, | ||
| + #[serde(default)] | ||
| + pub variables: serde_json::Value, | ||
| + #[serde(default)] | ||
| + pub operation_name: Option<String>, | ||
| + #[serde(default)] | ||
| + pub state_ref: Option<String>, | ||
| + #[serde(default)] | ||
| + pub ref_path: Option<String>, | ||
| + } | ||
| + | ||
| + /// Standard GraphQL response envelope. | ||
| + #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| + pub struct GraphQlResponse { | ||
| + pub data: Option<serde_json::Value>, | ||
| + #[serde(default)] | ||
| + pub errors: Vec<serde_json::Value>, |
There was a problem hiding this comment.
The api module behind the server feature uses serde_json::Value for request variables and response bodies, but crates/gatos-graphql/Cargo.toml declares only serde as an optional dependency. Compiling the crate with --features server will fail with use of undeclared crate or module 'serde_json'. Declare serde_json (probably optional) and include it in the server feature to make the new structs buildable.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 101
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
README.md (1)
200-200: Update legacy "message bus" terminology to "message plane".Line 200 still refers to the "message bus" in the research profile description. Update to "message plane" to align with the terminology shift throughout the PR.
-Research Profile defaults: PoF required on state pushes; `refs/gatos/policies/**`, `refs/gatos/state/**`, and `refs/gatos/audit/**` are fast-forward only; message bus rotates shards at ~100k msgs or ~192 MB with 30-day TTL; audit proofs + latest state are GC anchors; opaque pointers publish commitments while private bytes live behind a policy-gated resolver. See [docs/research-profile.md](./docs/research-profile.md). +Research Profile defaults: PoF required on state pushes; `refs/gatos/policies/**`, `refs/gatos/state/**`, and `refs/gatos/audit/**` are fast-forward only; message plane rotates shards at ~100k msgs or ~192 MB with 30-day TTL; audit proofs + latest state are GC anchors; opaque pointers publish commitments while private bytes live behind a policy-gated resolver. See [docs/research-profile.md](./docs/research-profile.md).docs/guide/HELLO-PRIVACY.md (1)
3-11: Remove duplicate anchor IDs that violate HTML specification.Lines 3-11 contain multiple duplicate anchor IDs (
hello-gatos--data-privacy-pathappears 3 times, plus variants). This violates the HTML specification requirement thatidattributes must be unique within a document. Browsers will ignore duplicates, breaking internal navigation.The same issue repeats throughout the document (lines 33-43, 51-61, etc.).
Remove all duplicate anchors and keep only one canonical ID per section. For example:
# Hello, GATOS — Data Privacy Path <a id="hello-gatos--data-privacy-path"></a> - -<a id="hello-gatos--data-privacy-path"></a> - -<a id="hello-gatos-data-privacy-path"></a> - -<a id="hello-gatos--data-privacy-path"></a> - -<a id="hello-gatos--data-privacy-path"></a><a id="hello-gatos-data-privacy-path"></a>docs/opaque-resolver.md (2)
32-47: Clarify relationship between Capability header and JWT cap claim.Lines 38 and 47 introduce two separate capability mechanisms (HTTP header and JWT claim) with no specification of their relationship:
- Must they match if both are present?
- Which takes precedence if they conflict?
- When is each required vs. optional?
- Line 38 references "policy" that demands capability, but no policy framework is defined
This ambiguity creates a security vulnerability—conflicting capability assertions could bypass authorization checks. A resolver cannot securely validate requests without clear precedence rules.
Add normative rules:
- Optional `scope` — dataset/namespace scope - Optional `cap` — capability URI assertion (mirrors the pointer's `capability`). + +**Capability validation:** +- If the Capability header is present, the `cap` JWT claim MUST match it exactly, or the request MUST be rejected with 401 Unauthorized +- If only the JWT `cap` claim is present, use that value +- If neither is present, the resolver MAY accept or reject based on pointer policy +- The validated capability URI MUST match the pointer's `capability` field
57-66: Fix redundant digest headers and strengthen client verification requirement.Lines 59-60 specify two digest headers (
Digest: blake3=<hex>andX-BLAKE3-Digest: <hex>) with no explanation of why both exist or which is canonical. This redundancy creates confusion and theX-prefix is deprecated per RFC 6648 (2012).Line 66 says clients "double-check" the digest, implying optional verification. This weakens the security model—clients MUST verify digest matching, not "double-check" as a nice-to-have.
Apply this diff:
200 OK -Digest: blake3=<hex> -X-BLAKE3-Digest: <hex> +Digest: blake3=<hex> Content-Type: application/octet-stream <bytes> -Servers MUST recompute `blake3(plaintext)` and only return success when the digest matches the pointer envelope. Clients double-check using the `Digest` header. +Servers MUST recompute `blake3(plaintext)` and only return 200 OK when the digest matches the pointer envelope's digest field. Clients MUST verify that the `Digest` response header matches the expected digest from the pointer; if not, clients MUST reject the response as corrupted or tampered.scripts/validate_schemas.sh (1)
62-66: Dead code:governance_policy.schema.jsonconditional logic is unreachable.Line 62 includes a conditional check for
schemas/v1/policy/governance_policy.schema.json, but this schema is not in the SCHEMAS array (line 37–58). The conditional on line 62 will never match during the loop on line 60, making it dead code.Either add
governance_policy.schema.jsonto the SCHEMAS array, or remove the conditional logic on line 62.Clarify the intent: add the schema to the array if it should be validated, or remove the conditional if it's handled elsewhere:
# Option 1: Add to SCHEMAS array SCHEMAS=( ... "schemas/v1/policy/governance_policy.schema.json" ) # Option 2: Remove dead conditional - if [[ "$schema" == "$AJV_COMMON_REF" || "$schema" == "schemas/v1/policy/governance_policy.schema.json" ]]; then + if [[ "$schema" == "$AJV_COMMON_REF" ]]; thendocs/FAQ.md (1)
303-324: Tighten Message Plane config/dedupe semantics and keep paths consistentA few nits that will bite implementers later:
- Config ref path drift
- In the shard‑map section you standardize on
refs/gatos/messages-config/<topic>.json(Line 303).- In the “Concrete next steps → Message Plane” bullet you drop the
refs/gatos/prefix and just saymessages-config/<topic>.json(Lines 934–935).Pick one canonical spelling and stick to it everywhere (including examples and CLI help), otherwise operators will be guessing which tree you meant.
- Dedupe key vs per‑segment checkpoints
You say:
- “Consumers read from both shard maps via
messages.readand persist checkpoints per topic/segment; dedupe by(topic, ulid).” (Line 321)If checkpoints are per topic/segment, but dedupe is only keyed by
(topic, ulid), you’re implicitly assuming ULIDs are globally unique across all segments of a topic. If that’s a real guarantee, it needs to be spelled out; if not, the dedupe key probably wants to include segment (or commit) as well.
- “Message Plane DNA” phrasing
“Message Plane DNA + proof envelopes v1” (Line 650) is cute but opaque; for a spec/FAQ aimed at integrators, consider briefly expanding what “DNA” means here (the envelope/layout + refs semantics from ADR‑0005), or rephrasing to something like “Message Plane layout + proof envelopes v1” for searchability.
- Use a versioned shard map + dual-write migration. - Store topic config at `refs/gatos/messages-config/<topic>.json`: + Use a versioned shard map + dual-write migration. + Store topic config at `refs/gatos/messages-config/<topic>.json`: - Consumers read from both shard maps via `messages.read` and persist checkpoints per topic/segment; dedupe by `(topic, ulid)`. + Consumers read from both shard maps via `messages.read` and persist checkpoints per topic/segment; dedupe by `(topic, segment, ulid)` (or document that ULIDs are globally unique per topic). - We meet: Message Plane DNA + proof envelopes v1. + We meet: Message Plane layout + proof envelopes v1 (per ADR‑0005). - - `messages-config/<topic>.json` with versioned shard maps + dual-write migration. + - `refs/gatos/messages-config/<topic>.json` with versioned shard maps + dual-write migration.Also applies to: 650-652, 693-696, 934-935
docs/guide/CHAPTER-010.md (1)
93-111: Fix inverted “low-entropy” guidance for opaque pointersThis paragraph currently says:
“Policy decides which fields (if any) are low-entropy enough to remain plain
blobptrs; everything sensitive becomes anopaque_pointerenvelope.”That’s backwards from a privacy perspective:
- Low‑entropy values (small enums, short IDs, common secrets) are exactly what you cannot safely protect with an unsalted digest, because they’re brute‑forceable.
- High‑entropy values (large random IDs, big model blobs) are the ones where
digest = blake3(plaintext)is reasonably safe to expose.Given you’ve just told readers the pointer stores the plaintext digest (
digest = blake3(plaintext)), the current wording will encourage the wrong behavior.-- Because only the digest and URIs live in Git, private data never enters the public repo. Policy decides which fields (if any) are low-entropy enough to remain plain `blobptr`s; everything sensitive becomes an `opaque_pointer` envelope. +- Because only the digest and URIs live in Git, private data never enters the public repo. Policy decides which fields (if any) are **high‑entropy enough** to remain plain `blobptr`s; any low‑entropy or otherwise sensitive fields MUST be wrapped in an `opaque_pointer` envelope.You might also want a one‑liner somewhere here that explicitly warns: “Do not rely on
blake3(plaintext)to hide low‑entropy secrets; treat the digest as public only for high‑entropy data.”Also applies to: 120-123
docs/guide/CHAPTER-006.md (1)
74-91: 🚨 CRITICAL: Fix Mermaid diagram syntax causing docs build failure.The pipeline reports: "Element is missing end tag. Build failed while processing CHAPTER-006.md during docs:build." This is blocking documentation generation.
The Mermaid sequenceDiagram block (lines 76–91) likely has a malformed fence or Vue processing issue. Ensure:
- Triple backticks properly delimit the diagram.
- No unclosed XML/Vue tags appear before or after the diagram.
- The
mermaidlanguage identifier is correctly lowercase.Common culprits in Vite-based doc builds: missing closing backticks, stray angle brackets in comments, or incomplete XML anchors in the preceding section headers (lines 73–75).
Propose this fix:
The entire lifecycle of a job is represented as a series of events in the GATOS ledger. This diagram shows the flow from a client's request to the final result being recorded. -```mermaid +\`\`\`mermaid sequenceDiagram participant Client participant GATOS (Ledger) @@ -88,7 +88,7 @@ Worker->>GATOS (Ledger): 8. Create Result commit -``` +\`\`\`Or verify that the Vite Vue plugin is configured to handle Mermaid blocks correctly in
.vuepress/config.jsordocs/.vitepress/config.ts. If the build is stripping Mermaid, add an explicit rule to preserve language identifiers.docs/TECH-SPEC.md (1)
27-44: 🚨 CRITICAL: Fix section numbering inconsistency in TOC.The TOC has a critical numbering error. After introducing "8. Local Enforcement..." (line 27), subsequent sections are misnumbered:
8. Local Enforcement (Watcher + Hooks) [line 27] 9. Observability [line 28] 10. CI & Cross-Platform Determinism [line 29] 10. Security [line 30] ← DUPLICATE! 11. Performance Guidance [line 31] ... 17. Local Enforcement (Watcher + Hooks) [line 44] ← DUPLICATE!This causes confusion (section 10 appears twice, section 17 duplicates section 8).
Likely intent: Insert Local Enforcement as a new section after section 7, pushing all downstream sections. Update the TOC to:
8. Local Enforcement (Watcher + Hooks) 9. Observability 10. CI & Cross-Platform Determinism 11. Security 12. Performance Guidance ... 17. Governance Engine 18. Sessions & PoX ToolingVerify that text section headers match TOC numbering (search for
## 9\.,## 10\.etc. in the file).#!/bin/bash # Check for section numbering in text rg "^## [0-9]+\." docs/TECH-SPEC.md | sort
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (17)
Cargo.lockis excluded by!**/*.lockdocs/diagrams/generated/docs_SPEC__0679d036ea__mermaid_19.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_SPEC__0679d036ea__mermaid_2.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_SPEC__0679d036ea__mermaid_9.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_TECH-SPEC__15850d53f4__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_TECH-SPEC__15850d53f4__mermaid_2.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__SPEC__mermaid_19.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__SPEC__mermaid_2.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__SPEC__mermaid_9.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__TECH-SPEC__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__TECH-SPEC__mermaid_2.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__diagrams__architecture__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs__diagrams__data_flow__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_diagrams_architecture__105fc24d87__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_diagrams_data_flow__559f0c180d__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_guide_CHAPTER-001__d51d557e71__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**docs/diagrams/generated/docs_guide_README__5e9e6b7c1c__mermaid_1.svgis excluded by!**/*.svg,!**/generated/**
📒 Files selected for processing (83)
CONTRIBUTING.md(1 hunks)Cargo.toml(1 hunks)README.md(4 hunks)ROADMAP.md(0 hunks)TASKLIST.md(1 hunks)api/graphql/schema.graphql(1 hunks)crates/gatos-graphql/Cargo.toml(1 hunks)crates/gatos-graphql/src/lib.rs(1 hunks)crates/gatos-message-plane/Cargo.toml(1 hunks)crates/gatos-message-plane/README.md(5 hunks)crates/gatos-message-plane/src/lib.rs(1 hunks)crates/gatos-mind/src/lib.rs(0 hunks)crates/gatosd/Cargo.toml(1 hunks)crates/gatosd/src/main.rs(2 hunks)crates/gatosd/src/message_plane.rs(1 hunks)docs/FAQ.md(5 hunks)docs/FEATURES.md(3 hunks)docs/ROADMAP.md(17 hunks)docs/SPEC.md(13 hunks)docs/TASKS.md(1 hunks)docs/TECH-SPEC.md(9 hunks)docs/USE-CASES.md(2 hunks)docs/cli/watch.md(1 hunks)docs/decisions/ADR-0001/DECISION.md(1 hunks)docs/decisions/ADR-0001/flyingrobots.md(2 hunks)docs/decisions/ADR-0003/DECISION.md(1 hunks)docs/decisions/ADR-0004/DECISION.md(1 hunks)docs/decisions/ADR-0005/DECISION.md(1 hunks)docs/decisions/ADR-0006/DECISION.md(1 hunks)docs/decisions/ADR-0007/DECISION.md(1 hunks)docs/decisions/ADR-0008/DECISION.md(1 hunks)docs/decisions/ADR-0009/DECISION.md(1 hunks)docs/decisions/ADR-0010/DECISION.md(1 hunks)docs/decisions/ADR-0011/DECISION.md(1 hunks)docs/decisions/ADR-0012/DECISION.md(1 hunks)docs/decisions/ADR-0013/DECISION.md(1 hunks)docs/decisions/ADR-0014/DECISION.md(1 hunks)docs/decisions/ADR-0015/DECISION.md(1 hunks)docs/decisions/ADR-0016/DECISION.md(1 hunks)docs/decisions/ARD-FEEDBACK.md(1 hunks)docs/decisions/README.md(1 hunks)docs/diagrams/architecture.md(1 hunks)docs/diagrams/data_flow.md(1 hunks)docs/exporter.md(1 hunks)docs/guide/CHAPTER-001.md(3 hunks)docs/guide/CHAPTER-005.md(1 hunks)docs/guide/CHAPTER-006.md(5 hunks)docs/guide/CHAPTER-007.md(1 hunks)docs/guide/CHAPTER-008.md(1 hunks)docs/guide/CHAPTER-010.md(5 hunks)docs/guide/CHAPTER-011.md(1 hunks)docs/guide/CHAPTER-013.md(1 hunks)docs/guide/HELLO-PRIVACY.md(3 hunks)docs/guide/README.md(3 hunks)docs/opaque-pointers.md(2 hunks)docs/opaque-resolver.md(3 hunks)docs/research-profile.md(2 hunks)examples/v1/api/command_envelope_min.json(1 hunks)examples/v1/api/graphql_state_mapping_min.json(1 hunks)examples/v1/api/stream_frame_sub.json(1 hunks)examples/v1/api/webhook_delivery_min.json(1 hunks)examples/v1/export/export_manifest_min.json(1 hunks)examples/v1/federation/mounts_min.json(1 hunks)examples/v1/message-plane/consumer_checkpoint_min.json(1 hunks)examples/v1/message-plane/event_envelope_min.json(1 hunks)examples/v1/privacy/opaque_pointer_min.json(1 hunks)examples/v1/state/proof_of_fold_envelope_min.json(1 hunks)examples/v1/watch/event_min.json(1 hunks)schemas/README.md(1 hunks)schemas/v1/api/command_envelope.schema.json(1 hunks)schemas/v1/api/graphql_state_mapping.schema.json(1 hunks)schemas/v1/api/stream_frame.schema.json(1 hunks)schemas/v1/api/webhook_delivery.schema.json(1 hunks)schemas/v1/export/export_manifest.schema.json(1 hunks)schemas/v1/federation/mounts.schema.json(1 hunks)schemas/v1/message-plane/consumer_checkpoint.schema.json(1 hunks)schemas/v1/message-plane/event_envelope.schema.json(1 hunks)schemas/v1/policy/locks.schema.json(1 hunks)schemas/v1/privacy/opaque_pointer.schema.json(1 hunks)schemas/v1/proofs/pox_envelope.schema.json(1 hunks)schemas/v1/state/proof_of_fold_envelope.schema.json(1 hunks)schemas/v1/watch/events.schema.json(1 hunks)scripts/validate_schemas.sh(2 hunks)
💤 Files with no reviewable changes (2)
- crates/gatos-mind/src/lib.rs
- ROADMAP.md
🧰 Additional context used
🧬 Code graph analysis (3)
crates/gatosd/src/main.rs (1)
crates/gatosd/src/message_plane.rs (2)
new(14-16)max_page_size(18-20)
crates/gatosd/src/message_plane.rs (1)
crates/gatos-message-plane/src/lib.rs (4)
validate_ulid_str(181-192)publish(153-154)read(160-165)persist_checkpoint(171-177)
crates/gatos-message-plane/src/lib.rs (1)
crates/gatosd/src/message_plane.rs (4)
new(14-16)publish(34-42)read(46-57)persist_checkpoint(61-72)
🪛 GitHub Actions: Pages: PR Preview
docs/guide/CHAPTER-006.md
[error] 74-74: vite:vue: Element is missing end tag. Build failed while processing CHAPTER-006.md during docs:build (npm run docs:build).
🪛 LanguageTool
docs/decisions/ADR-0010/DECISION.md
[uncategorized] ~29-~29: The official name of this software platform is spelled with a capital “H”.
Context: ...ternal mapping that logs an event under refs/gatos/events/github/<ulid>. 2. Command Triggers & UX ...
(GITHUB)
[uncategorized] ~46-~46: The official name of this software platform is spelled with a capital “H”.
Context: ...ayloads are normalized and committed to refs/gatos/events/github/<ulid> so the ledger contains a full a...
(GITHUB)
[uncategorized] ~48-~48: The official name of this software platform is spelled with a capital “H”.
Context: ...xamples: - pull_request.opened → events/ns=github, kind=pr.opened with PR metadata. ...
(GITHUB)
[uncategorized] ~49-~49: The official name of this software platform is spelled with a capital “H”.
Context: ... - pull_request_review.submitted → events/ns=github, kind=review.submitted. - These eve...
(GITHUB)
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...stream automation (Message Plane topics gatos.github.events). 5. **Attestations & Artifact...
(GITHUB)
[uncategorized] ~56-~56: The official name of this software platform is spelled with a capital “H”.
Context: ...fs/folds/. 7. **Configuration (.gatos/github.yaml`)** ```yaml repo: org: ...
(GITHUB)
[uncategorized] ~73-~73: The official name of this software platform is spelled with a capital “H”.
Context: ...tion every 90 days; automation triggers POST /api/v1/github/rotate to reload credentials. - Ins...
(GITHUB)
[uncategorized] ~80-~80: The official name of this software platform is spelled with a capital “H”.
Context: ...e Approvals** - Disabled by default. .gatos/github.yaml may enable bridging via: ```...
(GITHUB)
[uncategorized] ~95-~95: The official name of this software platform is spelled with a capital “H”.
Context: ... by default; operators can override via .gatos/github.yaml queues { weights: ... }. - Cro...
(GITHUB)
[uncategorized] ~96-~96: The official name of this software platform is spelled with a capital “H”.
Context: ...tems or 2 minutes of lag, the App emits gatos.github.queue.backpressure events and temporar...
(GITHUB)
[uncategorized] ~97-~97: The official name of this software platform is spelled with a capital “H”.
Context: ...ntil lag <30s. - Audit commits under refs/gatos/audit/github/queues/<repo>/<ulid> capture rebalanci...
(GITHUB)
docs/decisions/ARD-FEEDBACK.md
[style] ~136-~136: Using many exclamation marks might seem excessive (in this case: 7 exclamation marks for a text that’s 2546 characters long)
Context: ...ing & shared caches - [x] Resolved > [!WARNING]- Recommendation > > Specify...
(EN_EXCESSIVE_EXCLAMATION)
docs/decisions/ADR-0008/DECISION.md
[locale-violation] ~37-~37: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...off (1s, 5s, 30s, 5m) up to 5 attempts; afterwards the delivery is parked in a dead-letter...
(AFTERWARDS_US)
docs/decisions/ADR-0005/DECISION.md
[style] ~35-~35: This phrase is redundant. Consider using “outside”.
Context: ...itical piece of the system's data model outside of the core Git repository, potentially co...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Schema validation (v1)
- GitHub Check: Link Check (xtask links)
- GitHub Check: audit
| > **ADR-0006 preview:** Once `git gatos install-hooks` lands, use it instead of `scripts/setup-hooks.sh`. It installs the managed `pre-commit`, `pre-push`, and `post-checkout/merge` hooks referenced in the spec, plus records bypasses under `refs/gatos/audit/locks/*`. | ||
|
|
||
| ### Watcher / Lock Testing | ||
|
|
||
| - `git gatos watch --once` — run a single enforcement pass to verify `.gatos/policy.yaml` `locks[]` before committing docs. | ||
| - `git gatos lock acquire path1 path2` — exercise the governance workflow locally; grants appear under `refs/gatos/grants/...` and the watcher should clear read-only bits automatically. | ||
| - Event logs live in `~/.config/gatos/watch/events.log`; attach them to issues when debugging local enforcement. |
There was a problem hiding this comment.
🧩 Analysis chain
Verify CLI commands and event log paths are documented and implemented.
Lines 83-89 introduce new developer commands (git gatos watch --once, git gatos lock acquire) and a hardcoded event log path (~/.config/gatos/watch/events.log). These must be verified to exist in the implementation and be documented in the CLI help/man pages.
To verify these commands exist and are discoverable:
🏁 Script executed:
#!/bin/bash
# Search for CLI command definitions for watch and lock subcommands
rg -nP '(?:subcommand|command|Command)\s*.*(?:watch|lock)' -g '!*.md' --type rust | head -20
# Search for the hardcoded event log path
rg -n '\.config/gatos/watch/events\.log' -g '!*.md'
# Check if watch and lock are documented in help
rg -n 'fn.*watch|fn.*lock' --type rust | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Discover repository structure and primary language
echo "=== Repository Structure ==="
find . -maxdepth 2 -type f \( -name "*.rs" -o -name "*.go" -o -name "*.py" -o -name "*.ts" -o -name "*.js" -o -name "Cargo.toml" -o -name "go.mod" -o -name "package.json" \) | head -20
echo -e "\n=== Top-level files ==="
ls -la | grep -E '\.(rs|go|py|toml|mod|json)$|Makefile|Dockerfile'
echo -e "\n=== Broad search: 'watch' command ==="
rg -n 'watch' -g '!*.md' -g '!*.lock' -g '!*.txt' | head -15
echo -e "\n=== Broad search: 'lock' command ==="
rg -n 'lock' -g '!*.md' -g '!*.lock' -g '!*.txt' | head -15
echo -e "\n=== Search for event log path ==="
rg -n 'events\.log|watch.*log|gatos.*log' -g '!*.md' | head -10Length of output: 2642
🏁 Script executed:
#!/bin/bash
# Identify repository purpose
echo "=== README intro ==="
head -50 README.md 2>/dev/null | head -20
echo -e "\n=== Cargo.toml purpose ==="
head -20 Cargo.toml
echo -e "\n=== ADR-0006 references ==="
rg -n 'ADR-0006|adr-0006|adr0006' -g '!*.md' --no-heading
echo -e "\n=== Check for gatos implementation or references ==="
rg -n 'git.*gatos|gatos.*install|gatos.*lock|gatos.*watch' -g '*.md' | head -15
echo -e "\n=== Check CONTRIBUTING.md for context about these features ==="
rg -nB5 -A5 'gatos.*watch|gatos.*lock|gatos.*install-hooks' CONTRIBUTING.md | head -40
echo -e "\n=== Directory structure ==="
ls -la src/ 2>/dev/null || echo "[no src/ directory]"
ls -la .gatos/ 2>/dev/null || echo "[no .gatos/ directory]"Length of output: 5279
🏁 Script executed:
#!/bin/bash
echo "=== Searching crates for 'watch' command handler ==="
find crates -type f -name '*.rs' | xargs grep -l 'watch' | head -10
echo -e "\n=== Searching crates for 'lock' command handler ==="
find crates -type f -name '*.rs' | xargs grep -l 'lock.*acquire\|lock.*command' | head -10
echo -e "\n=== Checking CLI crate structure ==="
ls -la crates/*/src/ 2>/dev/null | grep -E '^d|^-' | head -20
echo -e "\n=== Search for event log path configuration ==="
find crates -type f -name '*.rs' | xargs grep -n 'events\.log\|watch.*log\|config.*gatos' 2>/dev/null | head -15
echo -e "\n=== Check main CLI entry (gatosd or similar) ==="
find crates -name 'main.rs' -o -name 'cli.rs' | xargs ls -la 2>/dev/null
echo -e "\n=== ADR-0006 full search (including non-.md) ==="
find . -type f \( -name '*.rs' -o -name '*.toml' -o -name '*.yaml' -o -name '*.json' \) | xargs grep -l 'ADR-0006\|adr.0006' 2>/dev/nullLength of output: 1494
🏁 Script executed:
#!/bin/bash
echo "=== gatosd main.rs content ==="
cat crates/gatosd/src/main.rs
echo -e "\n=== All Rust files in crates (check sizes) ==="
find crates -type f -name '*.rs' -exec wc -l {} \; | sort -rn | head -20Length of output: 1858
Remove or clarify aspirational commands in CONTRIBUTING.md lines 83-89; commands do not exist in implementation.
Verification confirms that git gatos watch --once, git gatos lock acquire, and the event log path ~/.config/gatos/watch/events.log are not implemented in the codebase. gatosd is a minimal stub (~45 lines) containing only a --stdio flag and an explicit TODO stating the JSONL RPC server "will be implemented in a subsequent iteration." No watch, lock, or install-hooks subcommands exist.
Lines 83-89 should either:
- Be moved to a "Planned Features" section with explicit caveats that commands are not yet available
- Be removed entirely until the commands are actually implemented and integrated into gatosd
The "ADR-0006 preview" prefix is insufficient to prevent developers from attempting to use these non-existent commands while following the contributing guide.
🤖 Prompt for AI Agents
In CONTRIBUTING.md around lines 83-89, the listed commands and log path are
aspirational and not implemented; update these lines by either removing them or
moving them to a new "Planned Features" section and add an explicit caveat that
they are not available yet (e.g., reference that gatosd is currently a minimal
stub and link to the TODO/issue tracking implementation), and change the
"ADR-0006 preview" prefix to a clear note that the commands are planned but not
implemented to avoid misleading contributors.
| license.workspace = true | ||
| repository.workspace = true | ||
| description = "Placeholder for GATOS Message Bus implementation" | ||
| description = "Placeholder for the GATOS Message Plane (message bus) implementation" |
There was a problem hiding this comment.
Outdated terminology in description contradicts package rename.
The description still says "message bus" in parentheses while the package name is "gatos-message-plane". This inconsistency muddies the nomenclature transition. Update the description to reflect the new terminology cleanly.
-description = "Placeholder for the GATOS Message Plane (message bus) implementation"
+description = "Placeholder for the GATOS Message Plane (event streaming) implementation"📝 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.
| description = "Placeholder for the GATOS Message Plane (message bus) implementation" | |
| description = "Placeholder for the GATOS Message Plane (event streaming) implementation" |
🤖 Prompt for AI Agents
In crates/gatos-message-plane/Cargo.toml around line 8, the package description
still uses the old term "message bus" which conflicts with the renamed package
"gatos-message-plane"; update the description text to remove or replace the
parenthetical "message bus" with terminology that matches the new name (e.g.,
"message plane" or simply omit the parenthetical) so the description
consistently reflects the package rename.
| <a id="adr-0003-consensus-governance-for-gated-actions"></a> | ||
|
|
||
| <a id="adr-0003-consensus-governance-for-gated-actions"></a> | ||
|
|
||
| <a id="adr-0003-consensus-governance-for-gated-actions"></a> |
There was a problem hiding this comment.
Remove duplicate anchor tags.
Lines 21–23 contain three identical anchor elements with the same id (adr-0003-consensus-governance-for-gated-actions). HTML id attributes must be unique; duplicate ids violate document structure and break anchor linking.
Apply this diff to remove the duplicates:
<a id="adr-0003-consensus-governance-for-gated-actions"></a>
-<a id="adr-0003-consensus-governance-for-gated-actions"></a>
-<a id="adr-0003-consensus-governance-for-gated-actions"></a>🤖 Prompt for AI Agents
In docs/decisions/ADR-0003/DECISION.md around lines 21 to 23, there are three
identical anchor tags with the same id
"adr-0003-consensus-governance-for-gated-actions"; remove the two duplicate
anchor elements so only a single anchor with that id remains, ensuring the id is
unique and no other surrounding content is changed.
| - Semantics: | ||
| - Default async: return 202 + `{ "ack": true, "job_id": "<ulid>", "state_ref": "<sha>" }` within 200 ms. | ||
| - Optional synchronous mode: when `sync=true` and the command’s budget <= 3s, return 200 + `{ "ok": true, "result": {...}, "state_ref": "<sha>" }`. | ||
| - `expect_state` enforces optimistic concurrency; if the current state head differs, respond 409 `EXPECT_STATE_MISMATCH` and include `current_state` in the body. |
There was a problem hiding this comment.
Undefined term "budget" confuses command execution model.
Line 30 says "when sync=true and the command's budget <= 3s, return 200". What is "budget"?
- Is it a per-command timeout?
- Who sets the budget (server? client? policy)?
- What happens if a command exceeds its budget (auto-cancel? return partial result)?
Clarify in Decision:
- **Synchronous mode budget**: Default 3s wall-clock timeout per command invocation.
- Clients may override via `sync_budget_ms` in command envelope (if policy allows).
- If timeout exceeded: return 202 + `{ack: true, job_id: "..."}` (fallback to async).
🤖 Prompt for AI Agents
In docs/decisions/ADR-0008/DECISION.md around line 30, the term "budget" is
undefined and confuses the command execution model—replace the ambiguous
sentence with a precise definition: define "Synchronous mode budget" as a
default 3s wall-clock timeout per command invocation, state that clients MAY
override it by providing sync_budget_ms in the command envelope where policy
permits (server enforces max/min per policy), and specify the behavior on
timeout (server should stop waiting and respond 202 with { ack: true, job_id:
"..." } to indicate fallback to async processing); update the decision text to
include these three points and any required examples or field names to make
behavior deterministic.
| - **Idempotency**: `request_id` **MUST** dedupe within a rolling 24h window. The server stores the last response keyed by `request_id`/actor; duplicates return the cached payload. | ||
| 2. **Result Plumbing**: Long work **SHALL** create a Job (ADR-0002) and stream progress on the Message Plane (ADR-0005) with trailers linking back to `request_id`. |
There was a problem hiding this comment.
Request_id deduplication strategy undefined; unbounded memory risk.
Line 31 mandates "request_id MUST dedupe within a rolling 24h window" but provides no implementation guidance:
- Where is the cache stored (Git? In-memory? Redis)?
- On server restart, is the cache lost?
- What's the max cache size? Can it grow unbounded and cause OOM?
- How is lookup indexed (O(1) hash? O(log n) B-tree)?
Without spec, implementations will be incompatible and vulnerable to resource exhaustion.
Add implementation details:
**Request_ID Deduplication**:
- Server maintains a rolling 24-hour cache of `(request_id, actor, response)` keyed by `(request_id, actor)`.
- Cache indexed via in-memory hash table; approximate size 1,000,000 entries × 8 KB = 8 GB max (configurable).
- On LRU eviction, oldest entries are discarded (entries older than 24h are always discarded).
- Duplicate requests: return cached response without re-executing the command.
🤖 Prompt for AI Agents
In docs/decisions/ADR-0008/DECISION.md around lines 31-32, the spec mandates 24h
request_id deduplication but lacks implementation details and resource
constraints; update the document to specify a concrete deduplication design:
state that the server maintains a rolling 24-hour cache of (request_id, actor,
response) keyed by (request_id, actor), stored in an in-memory hash table by
default with an option to use Redis for multi-instance persistence, document
that entries older than 24h are expired and LRU eviction enforces a configurable
maximum (e.g., default capacity and estimated memory footprint), describe lookup
as O(1) hash lookup, and note behavior on restart (in-memory lost; Redis
persisted) plus duplicate handling (return cached response without
re-execution).
| "properties": { | ||
| "kind": { "const": "bus.event" }, | ||
| "topic": { "type": "string" }, | ||
| "payload": { "type": "object" }, |
There was a problem hiding this comment.
Generic payload object lacks schema contract.
The busEvent.payload is typed as bare "type": "object" with no schema or documentation. How do consumers know what fields to expect? What if a malformed payload reaches clients?
Either define a schema for busEvent.payload or document that it is application-specific and consumers must handle unknown fields gracefully. If application-specific, recommend:
"payload": {
- "type": "object"
+ "type": "object",
+ "additionalProperties": true
}And add a comment above the schema or in documentation describing payload semantics.
📝 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.
| "payload": { "type": "object" }, | |
| "payload": { | |
| "type": "object", | |
| "additionalProperties": true | |
| }, |
🤖 Prompt for AI Agents
In schemas/v1/api/stream_frame.schema.json around line 71, the busEvent.payload
is currently typed as a bare "type": "object" with no schema or documentation;
update this by either (A) replacing the generic object with a concrete JSON
Schema describing expected fields, types, required properties and
additionalProperties behavior, or (B) add an explicit comment or documentation
block above the schema stating that payload is application-specific, include an
example payload, and set additionalProperties: true/false as appropriate and a
clear validation contract for consumers to follow so malformed payloads are
handled deterministically.
| "inputs_root": { "type": "string", "pattern": "^[a-z0-9]+:[a-f0-9]+$" }, | ||
| "program_id": { "type": "string" }, | ||
| "policy_root": { "type": "string" }, | ||
| "policy_code_root": { "type": "string" }, | ||
| "outputs_root": { "type": "string", "pattern": "^[a-z0-9]+:[a-f0-9]+$" }, |
There was a problem hiding this comment.
Pattern restrictiveness may not accommodate future digest algorithms.
The inputs_root and outputs_root patterns restrict to lowercase alphanumeric + colon + hex (^[a-z0-9]+:[a-f0-9]+$). If future digest schemes use uppercase, multi-colon notation, or non-hex characters (e.g., blake3 using base32), this will reject valid values. Consider relaxing or documenting the scope.
🤖 Prompt for AI Agents
In schemas/v1/proofs/pox_envelope.schema.json around lines 20-24 the regex for
inputs_root and outputs_root is overly restrictive (only lowercase alphanumerics
+ colon + hex) and will reject future digest formats; change the schema to stop
enforcing that exact hex-only pattern — either remove the pattern constraint and
add a clear description documenting the expected "algorithm: digest" form, or
replace it with a permissive pattern that allows uppercase letters and non-hex
characters (e.g., alphanumeric, hyphen/plus/underscore, and colon) so formats
like base32/blake3 are accepted; update the schema comment or description to
specify that the left side is an algorithm identifier and the right side is the
digest string, and add tests validating representative future digest examples.
🧩 Analysis chain
Underspecified string fields risk arbitrary injection.
Fields program_id, policy_root, and policy_code_root lack pattern constraints. Per ADR-0014, policy_root should be a commit ref, and program_id should reference the engine/artifact. Define explicit patterns or ref formats to prevent oversized or malformed values.
Suggest adding format constraints:
- "program_id": { "type": "string" },
- "policy_root": { "type": "string" },
- "policy_code_root": { "type": "string" },
+ "program_id": { "type": "string", "pattern": "^[a-z0-9._/-]+:[a-f0-9]{64}$" },
+ "policy_root": { "type": "string", "pattern": "^refs/gatos/policies/.*" },
+ "policy_code_root": { "type": "string", "pattern": "^[a-f0-9]{40}$" },🏁 Script executed:
#!/bin/bash
# Read the actual schema file
echo "=== SCHEMA FILE CONTENT ==="
cat schemas/v1/proofs/pox_envelope.schema.json 2>/dev/null || echo "File not found"
echo ""
echo "=== LOOKING FOR ADR FILES ==="
find . -type f -name "*.md" -o -name "*.txt" | xargs grep -l "ADR-0014\|ADR-0016" 2>/dev/null | head -20
echo ""
echo "=== SEARCHING FOR DECISION.MD ==="
find . -type f -name "DECISION.md" -o -name "decision.md" | head -5
echo ""
echo "=== ALL SCHEMA FILES IN CODEBASE ==="
find ./schemas -type f -name "*.json" 2>/dev/null | head -20Length of output: 2920
🏁 Script executed:
#!/bin/bash
# Search for usage of program_id, policy_root, policy_code_root in codebase
echo "=== USAGE OF program_id ==="
rg -n "program_id" --type json --type ts --type js -C 2 | head -50
echo ""
echo "=== USAGE OF policy_root ==="
rg -n "policy_root" --type json --type ts --type js -C 2 | head -50
echo ""
echo "=== USAGE OF policy_code_root ==="
rg -n "policy_code_root" --type json --type ts --type js -C 2 | head -50Length of output: 4303
🏁 Script executed:
#!/bin/bash
# Search for host:hash or similar patterns in existing schemas
echo "=== PATTERNS IN OTHER SCHEMAS (host-colon-hex patterns) ==="
rg -n "pattern.*\^.*:.*\$" ./schemas --type json -C 1 | head -60
echo ""
echo "=== LOOKING FOR REFS PATTERN ==="
rg -n "refs/" ./schemas --type json -C 2 | head -40
echo ""
echo "=== COMMIT/SHA PATTERNS ==="
rg -n "a-f0-9\}" ./schemas --type json -C 1 | head -40Length of output: 5064
🏁 Script executed:
#!/bin/bash
echo "=== ADR-0014 DECISION.MD ==="
cat ./docs/decisions/ADR-0014/DECISION.md 2>/dev/null | head -200
echo ""
echo "=== ADR-0016 DECISION.MD ==="
cat ./docs/decisions/ADR-0016/DECISION.md 2>/dev/null | head -200Length of output: 10549
🏁 Script executed:
#!/bin/bash
echo "=== job_manifest.schema.json (to compare policy_root) ==="
cat ./schemas/v1/job/job_manifest.schema.json | grep -A 2 -B 2 "policy_root"
echo ""
echo "=== proof_of_fold_envelope.schema.json (to compare policy_root) ==="
cat ./schemas/v1/state/proof_of_fold_envelope.schema.json | grep -A 2 -B 2 "policy_root"
echo ""
echo "=== Examples of policy_root usage ==="
find ./examples -type f \( -name "*.json" \) -exec grep -l "policy_root" {} \;
echo ""
echo "=== Actual policy_root values in examples ==="
rg "policy_root" ./examples --type json -A 1 -B 1 | head -30Length of output: 1015
Underspecified fields introduce injection risk—but the proposed patterns contradict existing schemas and lack ADR basis.
The three fields (program_id, policy_root, policy_code_root) ARE dangerously unconstrained in pox_envelope.schema.json. However, the proposed fix is flawed:
-
policy_root conflict:
job_manifest.schema.jsonalready constrainspolicy_rootto^sha256:[0-9a-f]{64}$. The suggested pattern^refs/gatos/policies/.*contradicts this and would create a breaking schema inconsistency. -
program_id speculative: ADR-0016 mentions tagged unions (
wasm:,oci:,containerd:) but prescribes NO exact regex pattern. The proposed^[a-z0-9._/-]+:[a-f0-9]{64}$lacks evidence. -
policy_code_root unsubstantiated: The suggested
^[a-f0-9]{40}$pattern appears nowhere in ADRs or existing schemas.
Before adding constraints, establish alignment across schemas:
- Verify whether
policy_rootshould follow job_manifest'ssha256:format or the git commit format referenced in ADR-0014. - Extract the canonical
program_idformat from ADR-0016 implementation notes or establish a new constraint consistent with codebase conventions. - Define
policy_code_rootsemantics (git SHA1? blake3?) before proposing a pattern.
Do not commit regex patterns that contradict adjacent schemas and lack documented justification.
🤖 Prompt for AI Agents
In schemas/v1/proofs/pox_envelope.schema.json around lines 20-24 the fields
program_id, policy_root and policy_code_root are currently unconstrained and the
reviewer warns proposed regexes would conflict with existing schemas/ADRs;
instead of adding ad-hoc patterns, (1) determine the canonical formats by
checking job_manifest.schema.json (policy_root currently uses
^sha256:[0-9a-f]{64}$) and ADR-0014/ADR-0016 and the codebase for program_id and
policy_code_root semantics, (2) align and reuse the confirmed patterns from
those canonical sources or define and document new ADR-backed formats, and (3)
only then update pox_envelope.schema.json to reference the agreed regexes (or
$ref shared schema) so schemas remain consistent and justified — do not commit
new regex constraints until step (1) is completed and approved.
| "additionalProperties": false | ||
| }, | ||
| "sig_alg": { "type": "string", "enum": ["ed25519"] }, | ||
| "sig": { "type": "string" } |
There was a problem hiding this comment.
Signature format undefined; incompatible with ADR-0014's stated format.
Per ADR-0014, signatures follow the format ed25519:<sig>#engine:<engine_id>, but the schema defines sig as a bare unconstrained string. Enforce a format constraint to align with documentation and prevent misuse.
Suggest adding format validation:
- "sig": { "type": "string" }
+ "sig": { "type": "string", "pattern": "^[a-f0-9]+$", "minLength": 128 }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In schemas/v1/proofs/pox_envelope.schema.json around line 42, the "sig" property
is currently an unconstrained string but must conform to ADR-0014's
"ed25519:<sig>#engine:<engine_id>" format; update the schema to keep type
"string" but add a pattern validation that requires the value to begin with
"ed25519:", followed by the encoded signature token, then "#engine:" and an
engine identifier (alphanumeric plus -/_ allowed), and update any unit tests or
schema consumers to expect and validate this format.
| { | ||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | ||
| "$id": "https://gatos.io/schemas/v1/state/proof_of_fold_envelope.schema.json", | ||
| "title": "Proof of Fold Envelope", | ||
| "type": "object", | ||
| "required": ["fold_id", "engine", "policy_root", "inputs", "output_shape_root", "metrics", "ts"], | ||
| "properties": { | ||
| "fold_id": { "type": "string", "pattern": "^blake3:[0-9a-f]{64}$" }, | ||
| "engine": { | ||
| "type": "object", | ||
| "required": ["program", "version", "platform"], | ||
| "properties": { | ||
| "program": { "type": "string" }, | ||
| "version": { "type": "string" }, | ||
| "platform": { "type": "string" } | ||
| } | ||
| }, | ||
| "policy_root": { "type": "string" }, | ||
| "inputs": { | ||
| "type": "object", | ||
| "required": ["events", "upstreams"], | ||
| "properties": { | ||
| "events": { "type": "array", "items": { "type": "string" } }, | ||
| "upstreams": { "type": "array", "items": { "type": "string" } } | ||
| } | ||
| }, | ||
| "output_shape_root": { "type": "string", "pattern": "^blake3:[0-9a-f]{64}$" }, | ||
| "metrics": { | ||
| "type": "object", | ||
| "properties": { | ||
| "units": { "type": "integer" }, | ||
| "duration_ms": { "type": "integer" } | ||
| } | ||
| }, | ||
| "ts": { "type": "string", "format": "date-time" }, | ||
| "content_id": { "type": "string", "pattern": "^blake3:[0-9a-f]{64}$" } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding additionalProperties constraint.
The schema does not include an additionalProperties: false constraint at the root level. Without it, the schema allows unknown fields, which can mask data errors or API evolution issues. Add this constraint unless forward compatibility with unknown fields is intentional.
🤖 Prompt for AI Agents
In schemas/v1/state/proof_of_fold_envelope.schema.json around lines 1-38, the
root schema lacks an additionalProperties constraint so unknown fields are
currently allowed; add "additionalProperties": false at the root object to
reject unexpected fields and, for stricter validation, add additionalProperties:
false to nested object schemas (e.g., engine, inputs, metrics) or explicitly
allow specific extras where forward compatibility is required.
| "events": { "type": "array", "items": { "type": "string" } }, | ||
| "upstreams": { "type": "array", "items": { "type": "string" } } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Specify format constraints for events and upstreams arrays.
Lines 23-24 define events and upstreams as arrays of generic strings. These should either:
- Include pattern validation (e.g.,
refs/gatos/...patterns, blake3 hashes), or - Include a description/examples in the schema to clarify expected format
Without this, consumers cannot validate the data rigorously. Reference the example file (proof_of_fold_envelope_min.json) to extract expected patterns.
🤖 Prompt for AI Agents
In schemas/v1/state/proof_of_fold_envelope.schema.json around lines 23-24, the
events and upstreams are typed as generic string arrays; update them to enforce
the expected formats by either adding pattern validation (e.g., a regex for refs
like ^refs/gatos/[a-zA-Z0-9/_-]+$ and for blake3 hashes like ^[0-9a-f]{64}$ or
the exact length used in proof_of_fold_envelope_min.json) or by adding clear
description and example fields showing the expected ref and hash formats;
extract the exact patterns/lengths from proof_of_fold_envelope_min.json and
apply them to the schema items (or include example arrays) so consumers can
validate values rigorously.
| "crates/gatos-message-plane", | ||
| "crates/gatos-graphql", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that new crate members exist and all dependents are updated.
The workspace members have been updated correctly in structure. However, this change must be validated against the actual crate directories and all downstream dependencies.
Verify:
- Both new crate directories (
crates/gatos-message-plane/andcrates/gatos-graphql/) exist with validCargo.tomlfiles. - All workspace members that previously depended on
gatos-mindare now updated to depend ongatos-message-plane.
🏁 Script executed:
#!/bin/bash
# Verify new crate members exist and have valid manifests
for crate in crates/gatos-message-plane crates/gatos-graphql; do
if [[ ! -f "$crate/Cargo.toml" ]]; then
echo "ERROR: Missing $crate/Cargo.toml"
exit 1
fi
echo "✓ $crate/Cargo.toml exists"
done
# Search for any remaining references to gatos-mind as a dependency
echo ""
echo "Searching for stray gatos-mind dependencies..."
rg -n 'gatos-mind' --type=toml crates/*/Cargo.toml
# Verify workspace members parse correctly
cd . 2>/dev/null && cargo metadata --format-version 1 >/dev/null 2>&1 && echo "✓ Workspace metadata valid" || echo "ERROR: Invalid workspace"Length of output: 160
🏁 Script executed:
#!/bin/bash
# Inspect the new crate Cargo.toml files
echo "=== crates/gatos-message-plane/Cargo.toml ==="
cat crates/gatos-message-plane/Cargo.toml
echo ""
echo "=== crates/gatos-graphql/Cargo.toml ==="
cat crates/gatos-graphql/Cargo.toml
echo ""
echo "=== Attempting cargo check for detailed errors ==="
cargo check 2>&1 | head -50
echo ""
echo "=== Checking for circular dependency issues ==="
rg -n 'gatos-message-plane|gatos-graphql' crates/gatos-message-plane/Cargo.toml crates/gatos-graphql/Cargo.tomlLength of output: 1347
Fix inconsistent workspace metadata inheritance in crates/gatos-graphql/Cargo.toml.
The new crate directories exist and gatos-mind has been cleanly removed from all dependencies. However, crates/gatos-graphql/Cargo.toml violates workspace consistency standards:
-
crates/gatos-message-plane/Cargo.tomlcorrectly uses workspace inheritance:version.workspace = true edition.workspace = true license.workspace = true
-
crates/gatos-graphql/Cargo.tomlhardcodes these values instead:version = "0.1.0" edition = "2021" license = "Apache-2.0"
Update crates/gatos-graphql/Cargo.toml to inherit version, edition, and license from the workspace root to maintain consistency and reduce maintenance burden.
🤖 Prompt for AI Agents
In Cargo.toml around lines 8 to 9 (crates/gatos-graphql/Cargo.toml), the crate
currently hardcodes version, edition, and license instead of inheriting
workspace metadata; change the crate's Cargo.toml to use workspace inheritance
by replacing the explicit version, edition, and license entries with the
corresponding workspace.workspace=true-style entries (i.e., set
version.workspace = true, edition.workspace = true, license.workspace = true) so
the crate picks these values from the workspace root and matches
crates/gatos-message-plane.
| [package] | ||
| name = "gatos-graphql" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
| license = "Apache-2.0" | ||
| description = "GraphQL gateway scaffolding for the GATOS project (ADR-0007)." | ||
|
|
||
| [dependencies] | ||
| async-trait = { version = "0.1", optional = true } | ||
| serde = { version = "1", features = ["derive"], optional = true } | ||
| thiserror = "1" | ||
|
|
||
| [features] | ||
| server = ["async-trait", "serde"] |
There was a problem hiding this comment.
🧩 Analysis chain
Dependency versions are loose; consider pinning or documenting strategy.
Versions specified as "0.1" and "1" are loose semver ranges (≥0.1.0, <0.2.0 and ≥1.0.0, <2.0.0). This is acceptable if:
- The project uses a
Cargo.lockcommitted to version control. - CI pins exact versions via a lockfile.
- The project accepts frequent minor/patch updates.
Recommendation: Either pin to specific versions (e.g., "0.1.3" or "1.0.50") or document the versioning strategy in the workspace's Cargo.toml or a contributing guide. Inconsistency across crates undermines reproducibility.
🏁 Script executed:
#!/bin/bash
# Check if Cargo.lock is committed
if [ -f Cargo.lock ]; then
echo "✓ Cargo.lock found (versions are pinned in version control)"
head -20 Cargo.lock
else
echo "⚠ Cargo.lock not committed; versions are not pinned"
fiLength of output: 492
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all Cargo.toml files and check versioning patterns
echo "=== All Cargo.toml files in workspace ==="
fd -n "Cargo.toml$" | head -20
echo -e "\n=== Check workspace root Cargo.toml (if exists) ==="
if [ -f "Cargo.toml" ]; then
cat Cargo.toml | grep -A 20 "\[workspace\]" || echo "No workspace section"
fi
echo -e "\n=== Look for versioning/contributing docs ==="
fd -n "(README|CONTRIBUTING|ADR|ADR-0007)" -e md -e txt | head -10
Length of output: 603
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all Cargo.toml files and check versioning patterns
echo "=== All Cargo.toml files in workspace ==="
find . -name "Cargo.toml" -type f | head -20
echo -e "\n=== Check workspace root Cargo.toml (if exists) ==="
if [ -f "Cargo.toml" ]; then
cat Cargo.toml
else
echo "No workspace root Cargo.toml"
fi
echo -e "\n=== Look for documentation files ==="
find . -maxdepth 2 \( -name "README*" -o -name "CONTRIBUTING*" -o -name "ADR*" \) -type f | head -10
Length of output: 1967
🏁 Script executed:
#!/bin/bash
# Check if async-trait, serde, thiserror are in workspace.dependencies
echo "=== Workspace dependencies for key packages ==="
grep -E "async-trait|serde|thiserror" Cargo.toml
echo -e "\n=== Sample other crate Cargo.toml for comparison ==="
echo "--- gatos-echo ---"
cat ./crates/gatos-echo/Cargo.toml | grep -A 30 "\[dependencies\]" | head -15
echo -e "\n--- gatos-ledger-core ---"
cat ./crates/gatos-ledger-core/Cargo.toml | grep -A 30 "\[dependencies\]" | head -15
echo -e "\n--- Full gatos-graphql Cargo.toml for reference ---"
cat ./crates/gatos-graphql/Cargo.tomlLength of output: 1214
Update to tilde requirements per documented workspace versioning policy.
The workspace defines an explicit versioning strategy in root Cargo.toml: "Use tilde requirements (~) to allow patch/minor updates within a compatible series while avoiding surprise major bumps." However, gatos-graphql deviates by using loose ranges ("0.1", "1") instead of tilde constraints ("~0.1.x", "~1.0.x"). Additionally, serde is already defined in workspace.dependencies as "~1.0.228" but is overridden here with a loose "1" instead of using { workspace = true }.
Update to:
async-trait = { version = "~0.1", optional = true }serde = { workspace = true, optional = true }(or{ version = "~1.0.228", features = ["derive"], optional = true }if workspace inheritance of features is problematic)thiserror = "~1"
This aligns with workspace policy and matches the pattern used in other crates like gatos-ledger-core. Cargo.lock mitigates reproducibility risk regardless, but consistency with documented policy is essential for maintainability.
🤖 Prompt for AI Agents
In crates/gatos-graphql/Cargo.toml around lines 1-14, dependency version specs
violate the workspace tilde-version policy and override workspace serde; update
async-trait to use a tilde constraint (async-trait = { version = "~0.1",
optional = true }), change serde to use the workspace dependency (serde = {
workspace = true, optional = true } and keep features = ["derive"] here only if
workspace inheritance of features is problematic—otherwise remove
version/features and rely on workspace), and change thiserror to a tilde
constraint (thiserror = "~1"); ensure the [features] server array still
references the optional deps correctly.
| [dependencies] | ||
| async-trait = { version = "0.1", optional = true } | ||
| serde = { version = "1", features = ["derive"], optional = true } | ||
| thiserror = "1" | ||
|
|
||
| [features] | ||
| server = ["async-trait", "serde"] |
There was a problem hiding this comment.
🧩 Analysis chain
Non-optional thiserror dependency violates feature gate hygiene.
Line 11 declares thiserror = "1" without optional = true, making it a hard dependency even when the server feature is not enabled. However, thiserror is almost certainly only used in GraphQlError or similar types behind the server feature gate (per the AI summary).
Problem: A consumer who depends on gatos-graphql without the server feature still pays the thiserror compilation cost and cannot minimize their build.
Fix: Declare thiserror as optional and include it in the server feature gate:
- thiserror = "1"
+ thiserror = { version = "1", optional = true }
- server = ["async-trait", "serde"]
+ server = ["async-trait", "serde", "thiserror"]Then verify that all thiserror usage in src/lib.rs is behind #[cfg(feature = "server")] guards.
Verify the library's no-feature API surface:
🏁 Script executed:
#!/bin/bash
# Check if thiserror is gated by feature in src/lib.rs
if [ -f crates/gatos-graphql/src/lib.rs ]; then
echo "=== Checking thiserror usage in lib.rs ==="
rg 'use thiserror|thiserror::' crates/gatos-graphql/src/lib.rs -B 2 -A 2
echo ""
echo "=== Checking for #[cfg(feature = \"server\")] guards ==="
rg '#\[cfg\(feature.*server' crates/gatos-graphql/src/lib.rs -B 1 -A 1
else
echo "✗ src/lib.rs not found"
fiLength of output: 273
Verification confirms the critical issue: thiserror is used in public API without feature gating.
The script output shows GraphQlError is defined as pub enum with #[derive(Debug, thiserror::Error)] but has no #[cfg(feature = "server")] guard. The feature guard on pub mod api does not protect the public error type itself.
Result: all consumers pay the thiserror cost regardless of enabling the server feature.
Fix required:
- thiserror = "1"
+ thiserror = { version = "1", optional = true }
- server = ["async-trait", "serde"]
+ server = ["async-trait", "serde", "thiserror"]Then either:
- Move
GraphQlErrorbehind#[cfg(feature = "server")], OR - If
GraphQlErrormust stay public, wrap all its definitions and usage in the feature gate
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/gatos-graphql/Cargo.toml around lines 8–14, thiserror is declared as
an unconditional dependency while the review notes a public GraphQlError type
deriving thiserror::Error; to fix, either (A) move the GraphQlError definition
and any uses behind #[cfg(feature = "server")] so the type isn't in the public
API without the feature, or (B) keep GraphQlError public but add #[cfg(feature =
"server")] to the type, its derives and all places it is referenced, and then
make the thiserror dependency feature-gated (mark it optional = true in
Cargo.toml and add "thiserror" to the server feature list) so consumers who
don't enable server won't pull thiserror. Ensure all references, trait impls,
and module exports are adjusted to compile under the new cfg.
| + /// Parameters accepted by the GraphQL endpoint. | ||
| + #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| + pub struct GraphQlRequest { | ||
| + pub query: String, | ||
| + #[serde(default)] | ||
| + pub variables: serde_json::Value, | ||
| + #[serde(default)] | ||
| + pub operation_name: Option<String>, | ||
| + #[serde(default)] | ||
| + pub state_ref: Option<String>, | ||
| + #[serde(default)] | ||
| + pub ref_path: Option<String>, | ||
| + } | ||
| + | ||
| + /// Standard GraphQL response envelope. | ||
| + #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| + pub struct GraphQlResponse { | ||
| + pub data: Option<serde_json::Value>, | ||
| + #[serde(default)] | ||
| + pub errors: Vec<serde_json::Value>, | ||
| + #[serde(default)] | ||
| + pub state_ref_resolved: Option<String>, | ||
| + #[serde(default)] | ||
| + pub shape_root: Option<String>, | ||
| + } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Be explicit about GraphQL request/response shapes and defaults
A few structural choices here are going to leak into every caller:
-
variables: serde_json::Value+#[serde(default)]means the missing‑field case deserializes as JSONnull, not{}. The GraphQL spec expectsvariablesto be an object; representing it as untypedValue+nullby default forces every consumer to downcast and validate. -
data: Option<serde_json::Value>anderrors: Vec<serde_json::Value>are fine for a stub, but they also punt the GraphQL error shape into “arbitrary JSON”, which makes it harder to add typed helpers/bindings later.
If you want this crate to be a stable surface, it’s worth tightening the types now:
- pub struct GraphQlRequest {
- pub query: String,
- #[serde(default)]
- pub variables: serde_json::Value,
+ pub struct GraphQlRequest {
+ pub query: String,
+ /// GraphQL variables object; omitted or `null` → empty map.
+ #[serde(default)]
+ pub variables: serde_json::Map<String, serde_json::Value>,
@@
- pub struct GraphQlResponse {
- pub data: Option<serde_json::Value>,
- #[serde(default)]
- pub errors: Vec<serde_json::Value>,
+ pub struct GraphQlResponse {
+ /// GraphQL data payload; absent or `null` on pure-error responses.
+ pub data: Option<serde_json::Value>,
+ /// Standard GraphQL errors list; empty when no errors.
+ #[serde(default)]
+ pub errors: Vec<serde_json::Value>, // consider a dedicated error struct laterEven if you keep serde_json::Value for now, I’d at least document the invariants (variables must be an object; errors contain GraphQL error objects) so you can later introduce stronger types without surprising downstream code.
| +/// Minimal marker trait so downstream crates can depend on this crate before | ||
| +/// the real server lands. | ||
| pub trait GraphQlService { | ||
| + /// Executes a GraphQL request and returns the JSON response body. | ||
| + fn execute(&self, request: &str) -> Result<String, GraphQlError>; | ||
| +} | ||
| + | ||
| +/// Placeholder error type. | ||
| +#[derive(Debug, thiserror::Error)] | ||
| +pub enum GraphQlError { | ||
| + #[error("not yet implemented" )] | ||
| + NotImplemented, | ||
| +} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Decide now whether GraphQlService is async and part of the stable API
Right now:
GraphQlService::executeis synchronous and returns aStringbody.- The crate comment says this is a temporary stub so other crates can “start wiring dependencies”.
Given the rest of the stack is almost certainly async HTTP, you’re going to want an async execute (or at least a future‑returning one). Changing this trait later is a hard, breaking change for every crate that compiled against it.
I’d strongly suggest one of:
- Make the trait async from day one (behind the same
serverfeature if needed), or - Explicitly mark the trait as unstable/internal (e.g.,
#[doc(hidden)]) and keep it out of your public compatibility contract.
While you’re here, fix the tiny nit in the error attribute and leave room for more variants:
-pub trait GraphQlService {
- /// Executes a GraphQL request and returns the JSON response body.
- fn execute(&self, request: &str) -> Result<String, GraphQlError>;
-}
+pub trait GraphQlService {
+ /// Executes a GraphQL request and returns the JSON response body.
+ /// Intentionally synchronous placeholder; expect this trait to change.
+ fn execute(&self, request: &str) -> Result<String, GraphQlError>;
+}
@@
-#[derive(Debug, thiserror::Error)]
-pub enum GraphQlError {
- #[error("not yet implemented" )]
- NotImplemented,
-}
+#[derive(Debug, thiserror::Error)]
+pub enum GraphQlError {
+ #[error("not yet implemented")]
+ NotImplemented,
+ // Consider ExecutionError, SerializationError, etc. once the server lands.
+}If this trait is meant to be long‑lived and used by external crates, bite the bullet and shape the async story now; otherwise, make it very clear it’s a throwaway shim.
📝 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.
| +/// Minimal marker trait so downstream crates can depend on this crate before | |
| +/// the real server lands. | |
| pub trait GraphQlService { | |
| + /// Executes a GraphQL request and returns the JSON response body. | |
| + fn execute(&self, request: &str) -> Result<String, GraphQlError>; | |
| +} | |
| + | |
| +/// Placeholder error type. | |
| +#[derive(Debug, thiserror::Error)] | |
| +pub enum GraphQlError { | |
| + #[error("not yet implemented" )] | |
| + NotImplemented, | |
| +} | |
| /// Minimal marker trait so downstream crates can depend on this crate before | |
| /// the real server lands. | |
| pub trait GraphQlService { | |
| /// Executes a GraphQL request and returns the JSON response body. | |
| /// Intentionally synchronous placeholder; expect this trait to change. | |
| fn execute(&self, request: &str) -> Result<String, GraphQlError>; | |
| } | |
| /// Placeholder error type. | |
| #[derive(Debug, thiserror::Error)] | |
| pub enum GraphQlError { | |
| #[error("not yet implemented")] | |
| NotImplemented, | |
| // Consider ExecutionError, SerializationError, etc. once the server lands. | |
| } |
| "items": { | ||
| "type": "object", | ||
| "required": ["match", "rule"], | ||
| "properties": { | ||
| "match": { "type": "string", "description": "Glob relative to repo root" }, | ||
| "rule": { "type": "string", "description": "Governance rule id (ADR-0003)" }, | ||
| "read_only": { "type": "boolean", "default": true } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add strict validation: missing additionalProperties constraint on locks items.
The locks array items schema (lines 8-16) lacks an "additionalProperties": false constraint. This permits arbitrary extra fields in lock configurations, which could mask typos (e.g., "reead_only": false would be silently ignored) or allow injection of unexpected data.
For strict schema validation, every object type should explicitly control additional properties.
Apply this diff:
"items": {
"type": "object",
"required": ["match", "rule"],
"properties": {
"match": { "type": "string", "description": "Glob relative to repo root" },
"rule": { "type": "string", "description": "Governance rule id (ADR-0003)" },
"read_only": { "type": "boolean", "default": true }
- }
+ },
+ "additionalProperties": false
}📝 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.
| "items": { | |
| "type": "object", | |
| "required": ["match", "rule"], | |
| "properties": { | |
| "match": { "type": "string", "description": "Glob relative to repo root" }, | |
| "rule": { "type": "string", "description": "Governance rule id (ADR-0003)" }, | |
| "read_only": { "type": "boolean", "default": true } | |
| } | |
| } | |
| }, | |
| "items": { | |
| "type": "object", | |
| "required": ["match", "rule"], | |
| "properties": { | |
| "match": { "type": "string", "description": "Glob relative to repo root" }, | |
| "rule": { "type": "string", "description": "Governance rule id (ADR-0003)" }, | |
| "read_only": { "type": "boolean", "default": true } | |
| }, | |
| "additionalProperties": false | |
| } | |
| }, |
🤖 Prompt for AI Agents
schemas/v1/policy/locks.schema.json lines 8-17: the objects used as items in the
"locks" array currently allow arbitrary extra fields; add an
"additionalProperties": false entry inside the items object schema (alongside
"type", "required", and "properties") so that any unknown fields are rejected
and typos like "reead_only" will fail validation.
| "items": { | ||
| "type": "object", | ||
| "required": ["when", "match"], | ||
| "properties": { | ||
| "when": { "enum": ["on_save", "on_fold" ] }, | ||
| "match": { "type": "string" }, | ||
| "run": { | ||
| "type": "array", | ||
| "items": { "type": "string" }, | ||
| "description": "Local command to run with deterministic args" | ||
| }, | ||
| "run_job": { "type": "string", "description": "Job manifest id to enqueue" }, | ||
| "timeout_s": { "type": "integer", "minimum": 1, "default": 120 } | ||
| }, | ||
| "oneOf": [ | ||
| { "required": ["run"] }, | ||
| { "required": ["run_job"] } | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Add security constraints: bounded timeout and strict validation on task items.
Two issues with the task schema:
-
Unbounded timeout (line 36):
timeout_shasminimum: 1but no maximum. A misconfigured or malicious task could set timeout to 2147483647 seconds (~68 years), hanging the watcher indefinitely and blocking other tasks. -
Missing additionalProperties constraint (lines 24-42): Task items lack
"additionalProperties": false, allowing arbitrary fields and masking configuration errors.
Apply this diff:
"run_job": { "type": "string", "description": "Job manifest id to enqueue" },
- "timeout_s": { "type": "integer", "minimum": 1, "default": 120 }
+ "timeout_s": {
+ "type": "integer",
+ "minimum": 1,
+ "maximum": 86400,
+ "default": 120,
+ "description": "Task timeout in seconds (max 24 hours)"
+ }
},
"oneOf": [
{ "required": ["run"] },
{ "required": ["run_job"] }
- ]
+ ],
+ "additionalProperties": false
}🤖 Prompt for AI Agents
In schemas/v1/policy/locks.schema.json around lines 24 to 43, the task item
schema allows unbounded timeouts and arbitrary extra fields; set a sane upper
bound for "timeout_s" (for example "maximum": 86400) in the timeout_s property
and add "additionalProperties": false to the item object so only the defined
properties ("when","match","run","run_job","timeout_s") are allowed; keep the
existing "minimum":1 and the oneOf requirement intact.
| - [x] **ADR Coverage: Sessions & PoX** | ||
| - *Summary*: Author ADRs that define the Sessions feature set and the Proof-of-Experiment workflow so roadmap items have specs. | ||
| - *Problem Statement*: `docs/TASKS.md` references Sessions without any ADR, and PoX (M7) lacks a normative document; engineers cannot implement without an agreed contract. | ||
| - *Acceptance Criteria*: ✅ ADR-0015 (Sessions) and ADR-0016 (PoX) exist with diagrams + consequences; roadmap/task references updated. | ||
| - *Test Plan*: ✅ Markdown lint + `rg` show new ADR ids in README/ROADMAP. | ||
| - *LLM Prompt*: “You are drafting an Architecture Decision Record. Produce an ADR that specifies the GATOS Sessions feature (start/undo/fork/merge with lattice/DPO joins) aligned with existing policy/state planes, including decision, diagrams, and consequences.” |
There was a problem hiding this comment.
🧩 Analysis chain
Task 1 marked "checked" but ADR-0015 and ADR-0016 existence not verified in PR context.
Line 3 marks Task 1 as completed (✅ ADR Coverage: Sessions & PoX), claiming:
✅ ADR-0015 (Sessions) and ADR-0016 (PoX) exist with diagrams + consequences; roadmap/task references updated.
However, these ADRs are not present in the provided files for review. This creates a critical inconsistency: either (a) they exist in the PR but are not in scope, or (b) the checkbox is premature.
Recommendation: Do not mark this task as complete until:
docs/decisions/ADR-0015/DECISION.mdanddocs/decisions/ADR-0016/DECISION.mdare verified in the PR.- Both ADRs are cross-linked from README and other documentation.
- Key decisions (e.g., session expiry = 30 days, PoX workflow) are explicitly stated in the ADR, not just in narrative docs like CHAPTER-005.
If the ADRs do exist but are simply not in scope, update the checkbox note to clarify:
- [x] **ADR Coverage: Sessions & PoX** (ADR-0015 & ADR-0016 in docs/decisions/, see also CHAPTER-005 and TASKL…)Verify ADR existence:
🏁 Script executed:
#!/bin/bash
for adr in ADR-0015 ADR-0016; do
if [ -f "docs/decisions/$adr/DECISION.md" ]; then
echo "✓ $adr exists"
else
echo "✗ $adr not found"
fi
doneLength of output: 35
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if ADRs contain diagrams and consequences sections
echo "=== ADR-0015 Content Check ==="
head -50 "docs/decisions/ADR-0015/DECISION.md" | grep -E "(##|diagram|consequence|Decision|Status)" || echo "No clear sections detected"
echo ""
echo "=== ADR-0016 Content Check ==="
head -50 "docs/decisions/ADR-0016/DECISION.md" | grep -E "(##|diagram|consequence|Decision|Status)" || echo "No clear sections detected"
echo ""
echo "=== Cross-linking Check ==="
rg -n "ADR-0015|ADR-0016" docs/README.md docs/ROADMAP.md 2>/dev/null || echo "No references found in README/ROADMAP"
Length of output: 393
🏁 Script executed:
#!/bin/bash
# Alternative: check raw file structure
for adr in ADR-0015 ADR-0016; do
echo "=== File listing for $adr ==="
find "docs/decisions/$adr" -type f 2>/dev/null || echo "Directory not found"
doneLength of output: 139
---
🏁 Script executed:
```shell
#!/bin/bash
# Deep dive: Check full ADR files for Consequences section and verify cross-references
echo "=== ADR-0015 Full Search for Consequences ==="
grep -n -i "consequences\|consequences" "docs/decisions/ADR-0015/DECISION.md" || echo "⚠ No Consequences section found"
echo ""
echo "=== ADR-0016 Full Search for Consequences ==="
grep -n -i "consequences\|consequences" "docs/decisions/ADR-0016/DECISION.md" || echo "⚠ No Consequences section found"
echo ""
echo "=== All References to ADR-0015 in Docs ==="
rg -r "ADR-0015" docs/ --type md 2>/dev/null || echo "⚠ ADR-0015 not referenced anywhere in docs/"
echo ""
echo "=== All References to ADR-0016 in Docs ==="
rg -r "ADR-0016" docs/ --type md 2>/dev/null || echo "Complete reference list provided"
Length of output: 10838
---
🏁 Script executed:
```shell
#!/bin/bash
# CRITICAL: Verify if these corrupted paths are real files or broken references
echo "=== Checking if corrupted paths exist as actual files ==="
for path in "ADR-0015SPEC.md" "ADR-0016SPEC.md" "ADR-0015guide/README.md" "ADR-0016guide/README.md" "ADR-0015research-profile.md" "ADR-0016research-profile.md"; do
if [ -f "$path" ]; then
echo "✓ FOUND: $path"
else
echo "✗ MISSING (broken ref): $path"
fi
done
echo ""
echo "=== Sample corrupted TASKLIST.md line-by-line verification ==="
grep -n "ADR-0015TASKS\|ADR-0016TASKS" TASKLIST.md | head -3
echo ""
echo "=== Verify intended paths should exist ==="
ls -la docs/TASKS.md 2>/dev/null || echo "✗ docs/TASKS.md not found"
ls -la docs/decisions/ADR-0015/DECISION.md 2>/dev/null || echo "✗ ADR file missing"
Length of output: 593
Original review incorrectly concluded ADRs don't exist — they do. However, critical path corruption prevents task completion.
ADR-0015 and ADR-0016 exist in docs/decisions/ with proper structure (Scope, Rationale, Decision, Diagrams, Consequences sections verified). The original concern was unfounded.
However, verification exposed systemic document path corruption: references throughout the codebase concatenate ADR identifiers directly into filenames, creating broken links.
Examples of malformed paths (non-existent):
./ADR-0015SPEC.md(should be./SPEC.md)./ADR-0015guide/README.md(should be./guide/README.md)ADR-0015TASKS.mdin TASKLIST.md (should bedocs/TASKS.md)./ADR-0015research-profile.md(should be./research-profile.md)
Broken in:
- README.md (multiple doc links)
- TASKLIST.md (cross-references)
- CONTRIBUTING.md (diagram generation paths)
crates/gatos-message-plane/README.mdand other crate READMEs
Impact: Acceptance criterion "roadmap/task references updated" (TASKLIST.md line 6) is not met. Task 1 cannot remain marked ✅ until path corruption is resolved.
Fix by removing the erroneous ADR-0015/ADR-0016 prefix concatenation from all documentation paths.
| - [ ] **Message Plane Implementation** | ||
| - *Summary*: Build the segmented Git-backed pub/sub system defined in ADR-0005 so M3 can move from Proposed to Accepted. | ||
| - *Problem Statement*: ADR-0005 is still Proposed; without actual crates and tests, Job Plane and downstream integrations are blocked. | ||
| - *Acceptance Criteria*: (1) New crate/service exposes publish/subscribe APIs with `refs/gatos/messages/**` segment rotation; (2) Consumer checkpoints persist under `refs/gatos/consumers/**`; (3) Integration tests cover at-least-once delivery, rotation, and pruning; (4) ADR-0005 status flipped to Accepted. | ||
| - *Test Plan*: Rust unit tests for envelope serialization; end-to-end test spinning up two publishers/consumers verifying dedupe; regression test ensuring pruning respects checkpoints. | ||
| - *LLM Prompt*: “Implement a Git-backed message bus per ADR-0005: create a Rust module that writes message topics under `refs/gatos/messages/<topic>/<date>/<ulid>` with rotation at 100k messages or 192MB and checkpoints under `refs/gatos/consumers/...`. Include tests for publish, subscribe, rotation, and pruning.” |
There was a problem hiding this comment.
🧩 Analysis chain
ADR-0005 status ("Proposed") may block Task 2 implementation; clarify gating criteria.
Line 12 states:
ADR-0005 is still Proposed; without actual crates and tests, Job Plane and downstream integrations are blocked.
This implies ADR-0005's status is a gate for Task 2 to be accepted into the codebase. Clarify the definition of "accepted":
- Is an ADR considered "Accepted" once the code implementing it ships?
- Or must the ADR formally move from "Proposed" → "Accepted" status before code review?
- If the latter, does this PR include an ADR-0005 status update that we should verify?
Recommendation: Either (a) include an ADR-0005 status change in this PR and note it in the task acceptance, or (b) split Task 2 into two phases:
- Phase A: ADR-0005 status → Accepted (separate PR/review)
- Phase B: Implement crate (this PR)
Additionally, confirm that "Message Plane Implementation" (crate code) is NOT in scope for the current PR—if it is, this TASKLIST item is misdocumented (it should be in a future backlog, not the current PR).
I need to investigate the PR scope and ADR-0005 status to verify this review comment properly.
🏁 Script executed:
#!/bin/bash
# Find ADR-0005 file and check its status
fd -t f "ADR.*0005|adr.*0005" -iLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check git diff to see what changed in this PR
git diff HEAD~1 TASKLIST.md | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for Message Plane or message plane implementation code
fd -t f -i "message.*plane|plane.*message" | head -20Length of output: 34
🏁 Script executed:
#!/bin/bash
# Search for any ADR files and their status declarations
fd -t f "\.md$" -i | xargs grep -l "ADR-0005\|adr-0005" | head -10Length of output: 298
🏁 Script executed:
#!/bin/bash
# Check ADR-0005 status
cat docs/decisions/ADR-0005/DECISION.md | head -50Length of output: 2631
🏁 Script executed:
#!/bin/bash
# Check if message_plane.rs is substantial or stub
wc -l crates/gatosd/src/message_plane.rs
head -30 crates/gatosd/src/message_plane.rsLength of output: 835
🏁 Script executed:
#!/bin/bash
# Check git status and log
git statusLength of output: 195
🏁 Script executed:
#!/bin/bash
# Check current branch and recent commits
git log --oneline -n 10Length of output: 469
🏁 Script executed:
#!/bin/bash
# Get the actual TASKLIST.md content to verify it contains the snippet
grep -A 20 "Message Plane Implementation" TASKLIST.mdLength of output: 3079
The TASKLIST entry creates an unresolved gating criterion: clarify whether ADR-0005 status transitions are in scope or deferred.
The review comment is fully justified. Verification confirms:
- ADR-0005 is definitively "Proposed" (
docs/decisions/ADR-0005/DECISION.md, line 2) - No Message Plane implementation exists—only a 73-line stub in
crates/gatosd/src/message_plane.rsmarked "Placeholder service that will eventually wrap the real Git-backed Message Plane" - TASKLIST acceptance criterion (4) explicitly requires: "ADR-0005 status flipped to Accepted"
- Current PR does not include an ADR-0005 status change
The blocker is real: the TASKLIST task description states that ADR-0005 remaining "Proposed" blocks downstream work, yet the acceptance criteria itself requires flipping that status. This creates circular gating:
- Future implementation PR cannot be accepted until ADR-0005 is Accepted
- But ADR-0005 won't be Accepted until implementation is merged
- This PR (documentation backlog) doesn't resolve either side
Split this into two explicit tracks:
- Decision Track: Determine if ADR-0005 can move to "Accepted" based on specification alone (separate PR updating
docs/decisions/ADR-0005/DECISION.mdStatus field) - Implementation Track: Then implement the crate (references Decision Track PR)
Or, if the intent is "ADR approved once code ships," then reword acceptance criterion (4) to state: "After merge: ADR-0005 status updated to Accepted in follow-up" with explicit link to the separate decision PR.
🤖 Prompt for AI Agents
In TASKLIST.md around lines 10–15, the acceptance criterion (4) incorrectly
requires ADR-0005 to be flipped to Accepted as part of the implementation task,
creating a circular gate; fix by splitting into two explicit tracks or by
deferring the ADR status change: either (A) add a Decision Track item and a
clear instruction to open a separate PR that updates
docs/decisions/ADR-0005/DECISION.md Status to Accepted (include acceptance
requirements for that decision PR), and keep the Implementation Track focused on
building the crate/service and tests, or (B) change acceptance criterion (4) to
state “ADR-0005 status updated to Accepted in a follow-up decision PR after
implementation” and add a link to the follow-up PR template; update TASKLIST.md
to reflect the chosen approach and remove the requirement that the current PR
flips ADR-0005.
| - [ ] **Exporter CLI & Explorer-Root Verifier** | ||
| - *Summary*: Finish M6 by building the `gatos export` command and Explorer-Root verification flow spelled out in ADR-0011/docs/exporter.md. | ||
| - *Problem Statement*: Specs describe Parquet/SQLite exports and Explorer-Root, but no code produces or verifies artifacts, leaving analytics teams blocked. | ||
| - *Acceptance Criteria*: (1) CLI supports `gatos export --format {sqlite,parquet}` plus `--since/--until`, column filters, and predicates; (2) Outputs include manifest + explorer-root digest; (3) `gatos export verify <path>` recomputes digests and fails on mismatches; (4) Tests cover pushdown filters and manifest hashing. | ||
| - *Test Plan*: Golden tests exporting a fixture repo; fuzz tests on filter DSL; verification test that tampering data triggers failure. | ||
| - *LLM Prompt*: “Implement the `gatos export` CLI per ADR-0011: emit SQLite/Parquet datasets with explorer-root metadata and add a verification subcommand that recomputes the digest.” |
There was a problem hiding this comment.
🧩 Analysis chain
Exporter task references ADR-0011; verify ADR exists and is complete.
Line 25–29 (Task 4) describes:
Finish M6 by building the
gatos exportcommand ... spelled out in ADR-0011/docs/exporter.md.
Missing verification:
- Does
docs/decisions/ADR-0011/DECISION.mdexist? - Does
docs/exporter.mdexist and match the ADR? - Are the acceptance criteria (SQLite/Parquet export, explorer-root digest, verification CLI) actually specified in the ADR, or inferred from this task?
If ADR-0011 does not exist or is incomplete, the task cannot be implemented reliably—it becomes a specification-writing task disguised as an implementation task.
Verify ADR-0011 and exporter spec:
🏁 Script executed:
#!/bin/bash
# Check ADR-0011
if [ -f docs/decisions/ADR-0011/DECISION.md ]; then
echo "✓ ADR-0011 found"
rg -i "export|sqlite|parquet|explorer" docs/decisions/ADR-0011/DECISION.md -A 2 | head -20
else
echo "✗ ADR-0011 not found"
fi
# Check docs/exporter.md
if [ -f docs/exporter.md ]; then
echo "✓ docs/exporter.md found"
else
echo "⚠ docs/exporter.md referenced in task but not found"
fiLength of output: 1006
🏁 Script executed:
#!/bin/bash
# Get full ADR-0011 content
echo "=== Full ADR-0011 content ==="
cat docs/decisions/ADR-0011/DECISION.md
echo -e "\n=== Full docs/exporter.md content ==="
cat docs/exporter.mdLength of output: 8602
ADR-0011 exists and is comprehensive; however, it is marked Status: Draft—task cannot proceed against unapproved specification.
✓ Verified: docs/decisions/ADR-0011/DECISION.md exists and covers all acceptance criteria:
- CLI flags:
--format,--since,--until(Query Pushdown v1 section),--columns,--where - Manifest: export manifest conforming to
schemas/v1/export/export_manifest.schema.json - Explorer-Root digest: defined in docs/exporter.md with computation and verification semantics
gatos export verifysubcommand: specified in docs/exporter.md with failure modes
✓ Verified: docs/exporter.md exists and aligns with ADR, covering Explorer-Root normative spec, recommended artifact layout, and verification failure modes.
Status: Draft (dated 2025-11-09). Implementing against a Draft ADR without formal approval creates tech debt: later changes to the spec invalidate work, and no stakeholder sign-off exists. Task must wait for ADR approval or explicit authorization to proceed against an unapproved spec.
🤖 Prompt for AI Agents
In TASKLIST.md around lines 24-29, the Exporter CLI & Explorer-Root Verifier
task is marked to implement ADR-0011 but ADR-0011 is still Status: Draft; update
the task to reflect that work must not proceed until the ADR is approved or
explicit stakeholder sign-off is obtained: change the bullet to mark the task as
"Blocked — waiting on ADR-0011 approval", add a short rationale line referencing
the draft status and date (2025-11-09), and add an instruction to the acceptance
criteria or task metadata to proceed only after approval (or an explicit
approval flag/PR link). Ensure the task list and test plan remain, but clearly
flag blocking and next action (who to contact or how to record approval).
| /// Placeholder export so downstream builds keep working while the real API | ||
| /// is filled in. Remove once the Message Plane lands. | ||
| #[allow(clippy::must_use_candidate)] | ||
| pub const fn hello_message_plane() -> &'static str { | ||
| "Hello from gatos-message-plane!" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Why is this placeholder public?
The hello_message_plane() function is explicitly documented as a temporary placeholder (line 14: "Remove once the Message Plane lands"). Why expose this in the public API? If it's truly just scaffolding to satisfy downstream builds, it could be pub(crate) or feature-gated.
Consider making it non-public or feature-gated:
-/// Placeholder export so downstream builds keep working while the real API
-/// is filled in. Remove once the Message Plane lands.
-#[allow(clippy::must_use_candidate)]
-pub const fn hello_message_plane() -> &'static str {
- "Hello from gatos-message-plane!"
-}
+// TODO(ADR-0005): Remove this placeholder once the Message Plane lands
+#[cfg(test)]
+pub const fn hello_message_plane() -> &'static str {
+ "Hello from gatos-message-plane!"
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
crates/gatos-message-plane/src/lib.rs lines 13-18: the placeholder function
hello_message_plane() is unnecessarily public; change its visibility to
pub(crate) if it only exists to satisfy local/downstream builds, or gate it
behind a feature (e.g., add #[cfg(feature = "hello_message_plane")] and document
the feature in Cargo.toml) so it is not exposed by default, and update/remove
the inline comment to reflect the chosen approach and ensure downstream code
either uses the crate-local symbol or opts into the feature.
| if !value.get("payload").is_some() { | ||
| return Err(MessagePlaneError::InvalidEnvelope( | ||
| "missing 'payload'".into(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
UNNECESSARILY CONVOLUTED DOUBLE NEGATIVE.
Line 91 uses if !value.get("payload").is_some() which is a painful double negative. This hurts readability for absolutely no benefit.
- if !value.get("payload").is_some() {
+ if value.get("payload").is_none() {
return Err(MessagePlaneError::InvalidEnvelope(
"missing 'payload'".into(),
));
}Or even cleaner:
- if !value.get("payload").is_some() {
- return Err(MessagePlaneError::InvalidEnvelope(
- "missing 'payload'".into(),
- ));
- }
+ if !value.contains_key("payload") {
+ return Err(MessagePlaneError::InvalidEnvelope(
+ "missing 'payload'".into(),
+ ));
+ }📝 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.
| if !value.get("payload").is_some() { | |
| return Err(MessagePlaneError::InvalidEnvelope( | |
| "missing 'payload'".into(), | |
| )); | |
| } | |
| if value.get("payload").is_none() { | |
| return Err(MessagePlaneError::InvalidEnvelope( | |
| "missing 'payload'".into(), | |
| )); | |
| } |
🤖 Prompt for AI Agents
In crates/gatos-message-plane/src/lib.rs around lines 91 to 95, the conditional
uses a double negative (`if !value.get("payload").is_some()`) which is hard to
read; change it to a direct negative check like `if
value.get("payload").is_none()` (or use `if let None =
value.get("payload")`/`match`) and keep the existing error return unchanged so
the logic and behavior remain identical but readability improves.
| use gatos_message_plane::{ | ||
| validate_ulid_str, CheckpointStore, MessagePlaneError, MessagePublisher, MessageSubscriber, | ||
| TopicRef, | ||
| }; | ||
|
|
||
| use gatos_message_plane::{MessageEnvelope, PublishReceipt}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating imports.
The imports from gatos_message_plane are split across two use statements (lines 1-4 and line 6). While functionally correct, combining them would marginally improve readability.
-use gatos_message_plane::{
- validate_ulid_str, CheckpointStore, MessagePlaneError, MessagePublisher, MessageSubscriber,
- TopicRef,
-};
-
-use gatos_message_plane::{MessageEnvelope, PublishReceipt};
+use gatos_message_plane::{
+ validate_ulid_str, CheckpointStore, MessageEnvelope, MessagePlaneError, MessagePublisher,
+ MessageSubscriber, PublishReceipt, TopicRef,
+};📝 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.
| use gatos_message_plane::{ | |
| validate_ulid_str, CheckpointStore, MessagePlaneError, MessagePublisher, MessageSubscriber, | |
| TopicRef, | |
| }; | |
| use gatos_message_plane::{MessageEnvelope, PublishReceipt}; | |
| use gatos_message_plane::{ | |
| validate_ulid_str, CheckpointStore, MessageEnvelope, MessagePlaneError, MessagePublisher, | |
| MessageSubscriber, PublishReceipt, TopicRef, | |
| }; |
🤖 Prompt for AI Agents
In crates/gatosd/src/message_plane.rs around lines 1 to 6, the imports from
gatos_message_plane are split across two use statements; consolidate them into a
single use gatos_message_plane::{...} line listing validate_ulid_str,
CheckpointStore, MessagePlaneError, MessagePublisher, MessageSubscriber,
TopicRef, MessageEnvelope, and PublishReceipt to improve readability and reduce
duplication.
| impl MessageSubscriber for MessagePlaneService { | ||
| fn read( | ||
| &self, | ||
| _topic: &TopicRef, | ||
| since_ulid: Option<&str>, | ||
| limit: usize, | ||
| ) -> Result<Vec<PublishReceipt>, MessagePlaneError> { | ||
| if let Some(cursor) = since_ulid { | ||
| validate_ulid_str(cursor)?; | ||
| } | ||
| let _clamped = limit.clamp(1, MAX_PAGE_SIZE); | ||
| Ok(Vec::new()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Useless computation in stub.
Line 55 computes _clamped but immediately discards it. While the underscore indicates "intentionally unused," performing a computation that serves zero purpose is pointless and confusing.
Option 1 (preferred): Remove the unused computation:
fn read(
&self,
_topic: &TopicRef,
since_ulid: Option<&str>,
limit: usize,
) -> Result<Vec<PublishReceipt>, MessagePlaneError> {
if let Some(cursor) = since_ulid {
validate_ulid_str(cursor)?;
}
- let _clamped = limit.clamp(1, MAX_PAGE_SIZE);
+ // TODO(ADR-0005): Clamp limit to [1, MAX_PAGE_SIZE] and fetch messages
Ok(Vec::new())
}Option 2: Keep it if it demonstrates intended behavior:
- let _clamped = limit.clamp(1, MAX_PAGE_SIZE);
+ // Demonstrate intended clamping behavior for future implementation
+ let _clamped = limit.clamp(1, MAX_PAGE_SIZE);
+ // TODO(ADR-0005): Return up to _clamped messages
Ok(Vec::new())📝 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.
| impl MessageSubscriber for MessagePlaneService { | |
| fn read( | |
| &self, | |
| _topic: &TopicRef, | |
| since_ulid: Option<&str>, | |
| limit: usize, | |
| ) -> Result<Vec<PublishReceipt>, MessagePlaneError> { | |
| if let Some(cursor) = since_ulid { | |
| validate_ulid_str(cursor)?; | |
| } | |
| let _clamped = limit.clamp(1, MAX_PAGE_SIZE); | |
| Ok(Vec::new()) | |
| } | |
| } | |
| impl MessageSubscriber for MessagePlaneService { | |
| fn read( | |
| &self, | |
| _topic: &TopicRef, | |
| since_ulid: Option<&str>, | |
| limit: usize, | |
| ) -> Result<Vec<PublishReceipt>, MessagePlaneError> { | |
| if let Some(cursor) = since_ulid { | |
| validate_ulid_str(cursor)?; | |
| } | |
| // TODO(ADR-0005): Clamp limit to [1, MAX_PAGE_SIZE] and fetch messages | |
| Ok(Vec::new()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/gatosd/src/message_plane.rs around lines 45 to 58, the code computes
let _clamped = limit.clamp(1, MAX_PAGE_SIZE); then discards it; remove that
useless computation (delete the _clamped line) so the stub is not performing
pointless work — if you intended to enforce a clamped limit instead, replace the
unused binding with a used one (e.g., let clamped = limit.clamp(...)) and use
clamped in subsequent logic.
| - ../../../../schemas/v1/policy/locks.schema.json # TBD schema capturing `.gatos/policy.yaml` locks/watch config | ||
| - ../../../../schemas/v1/watch/events.schema.json # Structured event payloads emitted by the watcher (defined in implementation ADR) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify schema files exist or track their creation.
The comment indicates these are "TBD schema" files. Ensure these schema files are either included in this PR or tracked in an issue so the ADR references don't dangle indefinitely.
Run the following to check if the schemas exist:
🏁 Script executed:
#!/bin/bash
# Check if the referenced schema files exist
fd -t f "locks.schema.json|events.schema.json" schemas/Length of output: 71
Remove "TBD schema" comments—schemas are implemented.
The schema files exist and are committed to the repository. The inline comments claiming "TBD schema" are incorrect. Update lines 10–11 to remove misleading TBD designations since these schemas are no longer under-defined.
🤖 Prompt for AI Agents
In docs/decisions/ADR-0006/DECISION.md around lines 10 to 11, the two listed
schema entries wrongly include "TBD schema" comments; remove the "TBD schema"
phrases so the lines simply reference the committed schema files
(../../../../schemas/v1/policy/locks.schema.json and
../../../../schemas/v1/watch/events.schema.json) without implying they are
incomplete, and commit the updated file.
| - `message/envelope.json` — Canonical JSON conforming to `schemas/v1/message-plane/event_envelope.schema.json`. | ||
| - optional `message/attachments/<name>` blobs referenced via the envelope `refs` map (do not influence the canonical identifier). | ||
|
|
||
| Commits MUST include trailers `Event-Id: ulid:<26-char ULID>` (strictly monotonic per topic per publisher) and `Content-Id: blake3:<hex>` computed from the canonical envelope bytes. No additional files MAY appear at the tree root; implementations store extra artifacts under `message/attachments/`. |
There was a problem hiding this comment.
INCORRECT RFC 2119 KEYWORD.
Line 788 states "No additional files MAY appear at the tree root" but uses MAY incorrectly. Per RFC 2119, MAY indicates something is OPTIONAL. To express a prohibition, this MUST use "MUST NOT".
-Commits MUST include trailers `Event-Id: ulid:<26-char ULID>` (strictly monotonic per topic per publisher) and `Content-Id: blake3:<hex>` computed from the canonical envelope bytes. No additional files MAY appear at the tree root; implementations store extra artifacts under `message/attachments/`.
+Commits MUST include trailers `Event-Id: ulid:<26-char ULID>` (strictly monotonic per topic per publisher) and `Content-Id: blake3:<hex>` computed from the canonical envelope bytes. No additional files MUST NOT appear at the tree root; implementations MUST store extra artifacts under `message/attachments/`.📝 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.
| Commits MUST include trailers `Event-Id: ulid:<26-char ULID>` (strictly monotonic per topic per publisher) and `Content-Id: blake3:<hex>` computed from the canonical envelope bytes. No additional files MAY appear at the tree root; implementations store extra artifacts under `message/attachments/`. | |
| Commits MUST include trailers `Event-Id: ulid:<26-char ULID>` (strictly monotonic per topic per publisher) and `Content-Id: blake3:<hex>` computed from the canonical envelope bytes. No additional files MUST NOT appear at the tree root; implementations MUST store extra artifacts under `message/attachments/`. |
🤖 Prompt for AI Agents
In docs/SPEC.md around line 788, the sentence "No additional files MAY appear at
the tree root" uses the RFC2119 keyword MAY incorrectly for a prohibition;
change the keyword to "MUST NOT" so the sentence reads that no additional files
MUST NOT appear at the tree root (i.e., replace MAY with MUST NOT), and scan
nearby lines for any other RFC2119 keyword misuse to keep wording consistent
with the spec keyword policy.
| <a id="224-caching--rate-limits"></a> | ||
|
|
||
| - Responses include `shapeRoot` (commit-derived hash) and `stateRefResolved`. Historical `stateRef` responses MUST set `Cache-Control: immutable` + `ETag: <shapeRoot>`. `refPath` queries are `Cache-Control: no-cache` and clients should revalidate to see new commits. | ||
| - Default throttling: 600 requests / 60 seconds per actor. Servers MUST emit `X-RateLimit-Limit`, `X-RateLimit-Remaining`, and `Retry-After` headers when throttled. Policy rules may override limits per namespace/project. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Awkward rate limit phrasing.
Line 1640 states "600 requests / 60 seconds" which is needlessly verbose. Standard convention would be "600 requests per minute" or "10 requests per second". The current phrasing suggests someone couldn't decide which unit to use.
-Default throttling: 600 requests / 60 seconds per actor. Servers MUST emit `X-RateLimit-Limit`, `X-RateLimit-Remaining`, and `Retry-After` headers when throttled.
+Default throttling: 600 requests per minute per actor. Servers MUST emit `X-RateLimit-Limit`, `X-RateLimit-Remaining`, and `Retry-After` headers when throttled.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/SPEC.md around line 1640, replace the awkward "600 requests / 60
seconds" phrasing with a standard time unit; update it to "600 requests per
minute" (optionally include the equivalent "10 requests per second" in
parentheses) and keep the rest of the sentence unchanged so it now reads e.g.
"Default throttling: 600 requests per minute. Servers MUST emit
`X-RateLimit-Limit`, `X-RateLimit-Remaining`, and `Retry-After` headers when
throttled. Policy rules may override limits per namespace/project."
| ## 17. Local Enforcement (Watcher + Hooks) | ||
|
|
||
| <a id="tech-watcher"></a> | ||
|
|
||
| ### 8.1 Watcher Daemon (`gatos watch`) | ||
|
|
||
| - Process model: long-running daemon started via `gatos watch` or Auto-Start. Watches the workspace root and `.git/refs/gatos/**`. | ||
| - Platform integrations: | ||
| - macOS: FSEvents + `chflags uchg` for hardened locks. | ||
| - Linux: inotify + `chattr +i` optional; default `chmod -w`. | ||
| - Windows: ReadDirectoryChangesW + Win32 file attributes. | ||
| - State machine: | ||
| 1. Load `.gatos/policy.yaml` (`locks[]`, `watcher.tasks[]`). | ||
| 2. Compute lock table keyed by canonical path glob + rule id. | ||
| 3. For each locked path, determine Grant status by reading `refs/gatos/grants/**` (compare `Proposal-Id` + `rule`). | ||
| 4. Apply/clear read-only bit accordingly and persist snapshot to `~/.config/gatos/watch/state.json`. | ||
| 5. Emit JSONL events (schema: `schemas/v1/watch/events.schema.json`). Events are appended to `~/.config/gatos/watch/events.log` and optionally forwarded to notification centers. | ||
| - Automation hooks (`watcher.tasks[]`): when a save/fold event hits a matching glob, the watcher either runs a local command (`run`) or enqueues a Job Plane manifest (`run_job`). Concurrency cap defaults to 2 with per-task `timeout_s` config. | ||
|
|
||
| ### 8.2 Managed Hooks & CLI | ||
|
|
||
| - `gatos install-hooks` | ||
| - Detects OS/shell, writes shim scripts into `.git/hooks/`, and records metadata under `.git/gatos/hooks.json`. | ||
| - Hooks call back into `gatos hook run <name>` so logic lives in Rust. | ||
| - Hook behavior: | ||
| - `pre-commit`: ask watcher cache for lock status; reject staged diffs touching locked files without Grants. | ||
| - `pre-push`: recompute outbound refs, ensure Grants exist, verify PoF/PoE trailers per profile. Records success/failure under `refs/gatos/audit/policy/locks/<ulid>`. | ||
| - `post-merge`/`post-checkout`: reapply masks and trigger watcher rescan. | ||
| - CLI additions: | ||
| - `gatos lock acquire <paths...> [--wait/--no-wait] [--reason <text>]` — builds proposals (via governance API), polls Grants, prints table of results. | ||
| - `gatos lock release <paths...>` — revokes or supersedes grants. | ||
| - `gatos watch events --tail` — streams watcher JSONL output for debugging. | ||
| - Env overrides: `GATOS_NO_HOOKS=1` and `GATOS_NO_WATCH=1` are honored but print warnings and emit audit entries. | ||
|
|
||
| ### 8.3 Data Structures | ||
|
|
||
| - `schemas/v1/policy/locks.schema.json` | ||
| - `locks[]`: `{ match: string, rule: string, read_only?: bool }` | ||
| - `watcher`: `{ poll_fallback_ms?: integer, tasks?: [] }` | ||
| - `watcher.tasks[]`: `{ when: "on_save"|"on_fold", match: string, run?: string[], run_job?: string, timeout_s?: integer }` | ||
| - `schemas/v1/watch/events.schema.json` | ||
| - Base fields: `ts`, `actor`, `path`, `rule`, `action`, `grant_id?`, `proposal_id?`, `remediation?`. | ||
| - `action` enum includes `deny.write`, `lock.applied`, `lock.released`, `task.started`, `task.succeeded`, `task.failed`. | ||
| - Hooks log structure: `refs/gatos/audit/locks/<ulid>` commit tree contains `event.json` (canonical JSON) plus log attachments (stdout/ stderr) when tasks fire. | ||
|
|
||
| ### 8.4 Open Work | ||
|
|
||
| - Privilege boundaries: future ADR will define optional elevated helper for immutable flags on corporate-managed machines. | ||
| - Sandboxed automation: evaluate WASM job runners to avoid arbitrary shell execution for `watcher.tasks[].run`. | ||
| - UX: integrate with `gatos ui` for desktop notifications and lock browser. | ||
| #### Message Plane RPC: `messages.read` | ||
|
|
||
| The daemon exposes `messages.read` over the JSONL RPC channel so workers and bridges can page through commit-backed topics without parsing Git directly. | ||
|
|
||
| - **Input envelope** | ||
| - `method`: `messages.read` | ||
| - `params`: | ||
| - `topic` (string, required) — logical topic name (e.g., `governance`). | ||
| - `since_ulid` (string, optional) — resume cursor; server starts from oldest when omitted or unknown. | ||
| - `limit` (integer, default 128) — max messages to return (1–512 enforced). | ||
| - **Output envelope** | ||
| - `messages`: ordered list (oldest→newest). Each entry contains `ulid`, `commit`, `content_id`, `envelope_path` (always `message/envelope.json` unless overridden), and `canonical_json` (base64 of the canonical envelope bytes). | ||
| - `next_since`: ULID to use for the next page (empty array when fewer than `limit` rows remain). | ||
| - `checkpoint_hint` (optional): `{ "group": <string>, "topic": <string> }` so automated consumers can persist progress without issuing a second RPC. | ||
| - **Errors** | ||
| - `topic_not_found` (404) — topic ref missing. | ||
| - `invalid_ulid` (400) — malformed resume cursor. | ||
| - `limit_out_of_range` (409) — `limit < 1`. | ||
|
|
||
| `gatos-message-plane` is responsible for translating RPC calls to actual Git ref walks and enforcing ULID monotonicity per ADR-0005. | ||
|
|
||
| --- | ||
|
|
||
| ## 18. Sessions & PoX Tooling |
There was a problem hiding this comment.
🧩 Analysis chain
Align Local Enforcement section with ADR-0006 and verify schema references.
Lines 789–862 introduce the new Local Enforcement (Watcher + Hooks) section with subsections 8.1–8.4, followed by message.read RPC and Sessions & PoX subsections.
Structural concerns:
-
Subsection numbering: Subsections are labeled 8.1, 8.2, 8.3, 8.4, but then line 839 has "#### Message Plane RPC" (h4) and line 862 has "## 18. Sessions..." (h2). This breaks consistent hierarchy. Should these be:
- 8.5 Message Plane RPC, or
- Move to a separate section post-Local Enforcement?
-
Schema references: Lines 825–831 mention schemas:
schemas/v1/policy/locks.schema.jsonschemas/v1/watch/events.schema.json
Verify these exist in the PR. If not, they should be added or marked as TBD.
-
ADR alignment: Section 8 (Local Enforcement) should reference or be cross-referenced with ADR-0006. If ADR-0006 is not in this PR, clarify the relationship.
-
Audit event schema: Lines 832–833 mention hooks log structure and refs/gatos/audit/locks/. Is there a corresponding schema for lock audit events?
🏁 Script executed:
#!/bin/bash
# Check for referenced schemas
fd "locks\.schema|events\.schema|lock.*audit.*schema" schemas/
# Verify ADR-0006 exists
[ -f "docs/decisions/ADR-0006/DECISION.md" ] && echo "✓ ADR-0006 found" || echo "✗ ADR-0006 NOT found"
# Check for audit schema references
rg "refs/gatos/audit/locks" docs/Length of output: 1540
Fix subsection hierarchy: Message Plane RPC should be 8.5, not h4; clarify audit event schema.
Schemas and ADR-0006 verified present. However, the document structure contains a critical flaw:
-
Subsection numbering breaks after 8.4: "#### Message Plane RPC" (h4) sits between subsections 8.1–8.4 (h3) and "## 18. Sessions & PoX Tooling" (h2). Change to
### 8.5 Message Plane RPC (h3)or promote it to a separate section after Local Enforcement completes. -
Audit event schema clarification: Line 833 references
event.jsoninrefs/gatos/audit/locks/<ulid>, but no dedicated audit schema file exists. Clarify whether this intentionally reusesschemas/v1/watch/events.schema.jsonor if a new schema should be defined.
🤖 Prompt for AI Agents
In docs/TECH-SPEC.md around lines 789 to 862, the "Message Plane RPC" subsection
is mis-leveled (uses an h4) and breaks the 8.x numbering, and the audit event
schema is ambiguous; change the header to a level-3 subsection titled "8.5
Message Plane RPC" (or promote it to a distinct section immediately after 8.4)
so numbering continues correctly, and update the text around the audit log
(refs/gatos/audit/locks/<ulid>/event.json) to explicitly state whether it reuses
schemas/v1/watch/events.schema.json or point to a new dedicated audit schema
(add the schema filename/path and a one-sentence description); if a new schema
is required, add a brief note instructing creation of
schemas/v1/audit/locks.event.schema.json and reference it here.
| ## 18. Sessions & PoX Tooling | ||
|
|
||
| ### Sessions (ADR-0015) | ||
|
|
||
| - **RPC surface** | ||
| - `session.start { base_ref?, actor? }` → daemon validates actor key, resolves base ref (default `refs/heads/main`), generates ULID, creates `refs/gatos/sessions/<actor>/<ulid>` pointing to base, and writes metadata JSON. | ||
| - `session.checkpoint { session_id }` → daemon shells out to Git to create a commit with required trailers. | ||
| - `session.undo/fork/publish` map 1:1 to CLI subcommands; publish calls the Policy API using the aggregated diff. | ||
| - **Storage** | ||
| - Metadata file `.gatos/session/<ulid>.json` structure: | ||
| ```json | ||
| { | ||
| "session_id": "01H...", | ||
| "actor": "ed25519:...", | ||
| "base_ref": "refs/heads/main", | ||
| "policy_root": "<commit>", | ||
| "fold_root": "sha256:<hex>", | ||
| "created_at": "2025-11-18T18:00:00Z" | ||
| } | ||
| ``` | ||
| - Session-local folds live in `refs/gatos/sessions/<actor>/<ulid>/state/<ns>/<channel>` so Echo can diff shapes. | ||
| - **GC job** | ||
| - `gatos gc sessions` scans metadata, deletes sessions idle >30 days after logging to `refs/gatos/audit/sessions/<ulid>/expired`. | ||
|
|
||
| ### Proof-of-Experiment (ADR-0016) | ||
|
|
||
| - **Schema**: `schemas/v1/proofs/pox_envelope.schema.json` (RFC 8785). CLI ensures `program_id` accepts `wasm:`, `oci:`, or `exec:` prefixes. | ||
| - **Workflow** | ||
| 1. `pox create` collects pointer manifests (`inputs_root`), Explorer outputs, PoE/PoF links. | ||
| 2. `pox sign` signs BLAKE3 digest with ed25519; supports hardware keys via `gatos key use`. | ||
| 3. `pox publish` writes commit to `refs/gatos/audit/proofs/experiments/<ulid>`. | ||
| 4. `pox verify` checks signatures + linked proofs; if missing, returns structured errors. | ||
| 5. `reproduce` drives workers + folds; writes audit logs under `refs/gatos/audit/pox/<ulid>/repro/<run-ulid>`. | ||
| - **GitHub App**: exposes `gatos/pox` status check by calling `pox.status` RPC, ensuring experiments touching governed namespaces reference an existing PoX ULID. |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify structure and integration of Sessions & PoX section.
Lines 862–895 introduce "## 18. Sessions & PoX Tooling" as a new section. This raises questions about document organization:
-
TOC presence: Is "## 18. Sessions & PoX Tooling" included in the autogenerated TOC (lines 13–46)? If not, the TOC is incomplete. If yes, verify it appears in the correct position.
-
Integration with ADRs: This section should reference:
- ADR-0015 (Sessions) — verify it exists and matches descriptions (lines 864–885)
- ADR-0016 (PoX) — verify it exists and matches descriptions (lines 886–895)
-
RPC surface consistency: Lines 864–869 define
session.*RPC methods. Are these consistent with CHAPTER-006 (guide) and ADR-0015 (decision)? Cross-check:- session.start, session.checkpoint, session.undo, session.fork, session.publish (lines 867–869)
- Do these match ADR-0015 lines 27–31?
-
GC command: Line 884 mentions
gatos gc sessions. Is this documented in the CLI surface section (§15)? Should it be? -
Metadata structure: Lines 871–881 define session metadata JSON. Is there a corresponding schema (schemas/v1/sessions/metadata.schema.json)? If so, reference it.
🏁 Script executed:
#!/bin/bash
# Verify ADRs exist and match section descriptions
for adr in ADR-0015 ADR-0016; do
if [ -f "docs/decisions/$adr/DECISION.md" ]; then
echo "✓ $adr found — checking for session/pox references"
head -30 "docs/decisions/$adr/DECISION.md"
else
echo "✗ $adr NOT found"
fi
done
# Check if section 18 is in TOC
rg -n "18\. Sessions" docs/TECH-SPEC.md | head -5Length of output: 3799
🏁 Script executed:
#!/bin/bash
# 1. Check if section 18 is in TECH-SPEC TOC (lines 13-46)
echo "=== TOC Check (lines 13-46) ==="
sed -n '13,46p' docs/TECH-SPEC.md | grep -n "Sessions\|PoX" || echo "(No match in TOC)"
# 2. Check for session metadata schema
echo -e "\n=== Schema Files ==="
fd "session" docs/schemas 2>/dev/null || echo "(No session schema found)"
# 3. Check CLI surface section (§15) for gc sessions
echo -e "\n=== CLI Surface: gatos gc sessions ==="
rg -n "gc\s+sessions|gatos gc" docs/TECH-SPEC.md -B2 -A2
# 4. Check ADR-0015 for "publish" command
echo -e "\n=== ADR-0015: Full command list ==="
rg -n "session\s+(publish|start|undo|fork|checkpoint)" docs/decisions/ADR-0015/DECISION.mdLength of output: 1699
🏁 Script executed:
#!/bin/bash
# 5. Get full ADR-0015 lifecycle commands section
echo "=== ADR-0015 Lifecycle Commands (full) ==="
sed -n '/^## Lifecycle Commands/,/^## [^L]/p' docs/decisions/ADR-0015/DECISION.md | head -50Length of output: 42
Section 18 is missing from the TOC and the session metadata schema is unreferenced.
Three structural issues require fixes:
-
Missing TOC entry: Section 18 does not appear in lines 13–46 of TECH-SPEC.md. Add "18. Sessions & PoX Tooling" to the table of contents in correct numerical order.
-
Unreferenced metadata schema: Lines 871–881 define session metadata JSON structure (
.gatos/session/<ulid>.json) but no corresponding schema file exists atschemas/v1/sessions/metadata.schema.json. Either create this schema or add an explicit reference to existing validation logic. -
GC command documentation placement: Line 884 (
gatos gc sessions) is documented only in §18. Verify whether this should also be explicitly listed in §15 (CLI surface) for consistency with other subcommands, or confirm §18 subsumes CLI documentation for these features.
Verified correct: RPC methods (start, checkpoint, undo, fork, publish) and ADR references (ADR-0015 lines 27–31, ADR-0016) are accurate and consistent with TECH-SPEC lines 864–895.
🤖 Prompt for AI Agents
In docs/TECH-SPEC.md around lines 862–895: Section "18. Sessions & PoX Tooling"
is missing from the document TOC and the session metadata JSON is unreferenced,
plus the GC CLI entry is only documented here; update the file as follows — (1)
add a TOC entry "18. Sessions & PoX Tooling" in the table of contents between
the existing entries for 17 and 19 (lines ~13–46) so numbering and links
resolve; (2) either add a new JSON Schema file at
schemas/v1/sessions/metadata.schema.json matching the example fields
(session_id, actor, base_ref, policy_root, fold_root, created_at) and reference
it inline in §18, or add a precise cross-reference to the existing validation
logic that will be used (include the schema path or function name); (3) mirror
or reference the `gatos gc sessions` CLI entry in §15 (CLI surface) to keep CLI
commands grouped consistently, or add a note in §15 pointing to §18 if you
intend §18 to be the authoritative CLI documentation — apply whichever approach
maintains consistency with other commands and update any internal links.
| "location": { | ||
| "type": "string", | ||
| "description": "URI indicating where to retrieve the private blob.", | ||
| "format": "uri", | ||
| "pattern": "^(gatos-node://ed25519:[0-9a-fA-F]{64}|file:///|https://|s3://|ipfs://).*" | ||
| }, | ||
| "capability": { | ||
| "type": "string", | ||
| "description": "URI indicating how to authorize/decrypt the private blob.", | ||
| "format": "uri", | ||
| "pattern": "^(gatos-key://v1/aes-256-gcm/|kms://aws/|age://|sops://).*" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify URI pattern scope and add schema examples.
Lines 34–44 define location and capability URI patterns with permissive .* suffixes:
locationpattern:^(gatos-node://ed25519:[0-9a-fA-F]{64}|file:///|https://|s3://|ipfs://).*capabilitypattern:^(gatos-key://v1/aes-256-gcm/|kms://aws/|age://|sops://).*
These patterns are intentionally loose (allowing arbitrary suffixes), which is reasonable for versioning and query parameters. However, this permissiveness should be documented:
- Constraint clarity: Add a schema description or comment noting that these patterns permit version/query extensions. Example: "Allows gatos-key://v1/aes-256-gcm/?version=2."
- Scheme enumeration: Are these the exhaustive schemes, or are others permitted? If extensible, add a note.
- Missing schemes: The opaque-pointers.md mentions only these schemes, but what about custom/plugin schemes (e.g.,
vault://,secrets-manager://)? Should the schema be more restrictive or more permissive?
Consider adding $examples to the schema for clarity:
{
"examples": [
{
"kind": "opaque_pointer",
"algo": "blake3",
"digest": "blake3:a665a45920422f9d417e4867efdc4fb8a04a1f3fff1fa07e998e86f7f7a27ae3",
"location": "s3://my-bucket/data/model.bin",
"capability": "gatos-key://v1/aes-256-gcm/key-001"
}
]
}