ADR-0005: Shiplog spec + schema alignment (pre-PR kill-checked) #60
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,12 @@ | ||||||||||||||||||||||||
| [package] | ||||||||||||||||||||||||
| name = "gatos-privacy" | ||||||||||||||||||||||||
| version = "0.1.0" | ||||||||||||||||||||||||
| edition = "2021" | ||||||||||||||||||||||||
|
Comment on lines
+1
to
+4
There was a problem hiding this comment. 🛠️ Refactor suggestion | 🟠 Major Missing package metadata. The manifest lacks essential fields:
Add metadata: [package]
name = "gatos-privacy"
version = "0.1.0"
edition = "2021"
+description = "GATOS privacy model: opaque pointers and low-entropy validation"
+license = "MIT OR Apache-2.0" # or your chosen license
+repository = "https://github.com/flyingrobots/gatos"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| [dependencies] | ||||||||||||||||||||||||
| serde = { workspace = true, features = ["derive"] } | ||||||||||||||||||||||||
| serde_json = { workspace = true } | ||||||||||||||||||||||||
| blake3 = { workspace = true } | ||||||||||||||||||||||||
| hex = { workspace = true } | ||||||||||||||||||||||||
| anyhow = { workspace = true } | ||||||||||||||||||||||||
|
There was a problem hiding this comment. 🧩 Analysis chainRedundant error handling crates. Both
Library crates usually only need Verify usage: If 🏁 Script executed: #!/bin/bash
# Check how anyhow is actually used in the crate
echo "=== anyhow usage in gatos-privacy ==="
rg -n --type=rust 'use anyhow|anyhow::' crates/gatos-privacy/src/
echo -e "\n=== thiserror usage ==="
rg -n --type=rust 'use thiserror|thiserror::' crates/gatos-privacy/src/
echo -e "\n=== Public error types ==="
rg -n --type=rust 'pub.*Error|pub enum.*Error' crates/gatos-privacy/src/Length of output: 326 Remove unused Script output confirms 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| thiserror = "1" | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Loose thiserror version constraint enables breaking changes. Using Pin to a specific minor version: -thiserror = "1"
+thiserror = "1.0"Or even better, use workspace version like the other dependencies: -thiserror = "1"
+thiserror = { workspace = true }Then define it in the workspace 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # gatos-privacy | ||
|
|
||
| Opaque Pointer types and helpers for the GATOS hybrid privacy model (ADR-0004). | ||
|
|
||
| Key types | ||
| - `OpaquePointer`: JSON-facing struct that mirrors `schemas/v1/privacy/opaque_pointer.schema.json`. | ||
| - `digest: Option<String>` — plaintext digest (may be omitted) | ||
| - `ciphertext_digest: Option<String>` — ciphertext digest | ||
| - `extensions.class = "low-entropy"` implies `ciphertext_digest` MUST be present and `digest` MUST be absent. | ||
|
|
||
| - `VerifiedOpaquePointer`: wrapper that enforces invariants during deserialization. | ||
| - Use this at trust boundaries to guarantee the low-entropy rules. | ||
|
|
||
| Validation | ||
| - After deserializing `OpaquePointer`, call `pointer.validate()` to enforce: | ||
| - At least one of `digest` or `ciphertext_digest` is present. | ||
| - Low-entropy class requires `ciphertext_digest` and forbids `digest`. | ||
|
|
||
| Examples | ||
| ```rust | ||
| use gatos_privacy::{OpaquePointer, VerifiedOpaquePointer}; | ||
|
|
||
| // 1) Verified wrapper enforces invariants automatically | ||
| let v: VerifiedOpaquePointer = serde_json::from_str(json)?; | ||
|
|
||
| // 2) Manual validation on the plain struct | ||
| let p: OpaquePointer = serde_json::from_str(json)?; | ||
| p.validate()?; | ||
| ``` | ||
|
|
||
| Canonicalization | ||
| - When computing content IDs or digests, serialize JSON with RFC 8785 JCS (performed by higher layers). | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| //! gatos-privacy — Opaque Pointer types and helpers | ||
| //! | ||
| //! This crate defines the JSON-facing pointer envelope used by the | ||
| //! hybrid privacy model (ADR-0004). The struct mirrors the v1 schema | ||
| //! in `schemas/v1/privacy/opaque_pointer.schema.json`. | ||
| //! | ||
| //! Canonicalization: when computing content IDs or digests, callers | ||
| //! MUST serialize JSON using RFC 8785 JCS. This crate intentionally | ||
| //! does not take a dependency on a specific JCS implementation to | ||
| //! keep the workspace lean; higher layers may provide one. | ||
|
|
||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::Value; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct OpaquePointer { | ||
| pub kind: Kind, | ||
| pub algo: Algo, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub digest: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub ciphertext_digest: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub size: Option<u64>, | ||
| pub location: String, | ||
| pub capability: String, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub extensions: Option<Value>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum Kind { | ||
| OpaquePointer, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum Algo { | ||
| Blake3, | ||
| } | ||
|
|
||
| impl OpaquePointer { | ||
| /// Validate invariants beyond serde schema mapping. | ||
| pub fn validate(&self) -> Result<(), PointerError> { | ||
| let has_plain = self.digest.as_ref().map(|s| !s.is_empty()).unwrap_or(false); | ||
| let has_cipher = self | ||
| .ciphertext_digest | ||
| .as_ref() | ||
| .map(|s| !s.is_empty()) | ||
| .unwrap_or(false); | ||
| if !(has_plain || has_cipher) { | ||
| return Err(PointerError::MissingDigest); | ||
| } | ||
| let low_entropy = self | ||
| .extensions | ||
| .as_ref() | ||
| .and_then(|v| v.get("class")) | ||
| .and_then(|c| c.as_str()) | ||
| .map(|s| s == "low-entropy") | ||
| .unwrap_or(false); | ||
| if low_entropy { | ||
| if !has_cipher { | ||
| return Err(PointerError::LowEntropyNeedsCiphertextDigest); | ||
| } | ||
| if has_plain { | ||
| return Err(PointerError::LowEntropyForbidsPlainDigest); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error, PartialEq, Eq)] | ||
| pub enum PointerError { | ||
| #[error("at least one of digest or ciphertext_digest is required")] | ||
| MissingDigest, | ||
| #[error("low-entropy class requires ciphertext_digest")] | ||
| LowEntropyNeedsCiphertextDigest, | ||
| #[error("low-entropy class forbids plaintext digest")] | ||
| LowEntropyForbidsPlainDigest, | ||
| } | ||
|
|
||
| /// A validated wrapper that enforces `OpaquePointer::validate()` during | ||
| /// deserialization. Prefer this type when accepting pointers from untrusted | ||
| /// inputs; it guarantees schema-level invariants at the boundary. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize)] | ||
| #[serde(transparent)] | ||
| pub struct VerifiedOpaquePointer(pub OpaquePointer); | ||
|
|
||
| impl<'de> Deserialize<'de> for VerifiedOpaquePointer { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let inner = OpaquePointer::deserialize(deserializer)?; | ||
| inner | ||
| .validate() | ||
| .map_err(serde::de::Error::custom)?; | ||
| Ok(Self(inner)) | ||
| } | ||
| } | ||
|
|
||
| impl core::ops::Deref for VerifiedOpaquePointer { | ||
| type Target = OpaquePointer; | ||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,23 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| use gatos_privacy::OpaquePointer; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| fn read_example(rel: &str) -> String { | ||||||||||||||||||||||||||||||||||||||||||||||
| let dir = env!("CARGO_MANIFEST_DIR"); | ||||||||||||||||||||||||||||||||||||||||||||||
| std::fs::read_to_string(format!("{}/../../examples/v1/{}", dir, rel)).unwrap() | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+6
There was a problem hiding this comment. Bare The Apply this diff to provide actionable error context: fn read_example(rel: &str) -> String {
let dir = env!("CARGO_MANIFEST_DIR");
- std::fs::read_to_string(format!("{}/../../examples/v1/{}", dir, rel)).unwrap()
+ let path = format!("{}/../../examples/v1/{}", dir, rel);
+ std::fs::read_to_string(&path)
+ .unwrap_or_else(|e| panic!("Failed to read {}: {}", path, e))
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||
| fn ciphertext_only_pointer_should_deserialize() { | ||||||||||||||||||||||||||||||||||||||||||||||
| // This example omits plaintext digest by design (low-entropy class) | ||||||||||||||||||||||||||||||||||||||||||||||
| let json = read_example("privacy/pointer_low_entropy_min.json"); | ||||||||||||||||||||||||||||||||||||||||||||||
| let ptr: Result<OpaquePointer, _> = serde_json::from_str(&json); | ||||||||||||||||||||||||||||||||||||||||||||||
| assert!(ptr.is_ok(), "ciphertext-only opaque pointer must deserialize"); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+8
to
+14
There was a problem hiding this comment. 🧹 Nitpick | 🔵 Trivial Test lacks negative assertions. This test verifies successful deserialization but doesn't validate that the resulting pointer satisfies low-entropy constraints (e.g., Strengthen the test: #[test]
fn ciphertext_only_pointer_should_deserialize() {
// This example omits plaintext digest by design (low-entropy class)
let json = read_example("privacy/pointer_low_entropy_min.json");
- let ptr: Result<OpaquePointer, _> = serde_json::from_str(&json);
- assert!(ptr.is_ok(), "ciphertext-only opaque pointer must deserialize");
+ let ptr: OpaquePointer = serde_json::from_str(&json)
+ .expect("ciphertext-only opaque pointer must deserialize");
+ assert!(ptr.digest.is_none(), "low-entropy pointer must omit plaintext digest");
+ assert!(ptr.ciphertext_digest.is_some(), "low-entropy pointer must include ciphertext_digest");
+ assert_eq!(
+ ptr.extensions.as_ref().and_then(|e| e.get("class")).and_then(|v| v.as_str()),
+ Some("low-entropy"),
+ "low-entropy pointer must have extensions.class='low-entropy'"
+ );
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||
| fn both_digests_allowed_when_not_low_entropy() { | ||||||||||||||||||||||||||||||||||||||||||||||
| let json = read_example("privacy/opaque_pointer_min.json"); | ||||||||||||||||||||||||||||||||||||||||||||||
| let ptr: OpaquePointer = serde_json::from_str(&json).unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||||
| let has_digest = ptr.digest.as_ref().map(|s| !s.is_empty()).unwrap_or(false); | ||||||||||||||||||||||||||||||||||||||||||||||
| assert!(has_digest); | ||||||||||||||||||||||||||||||||||||||||||||||
| assert!(ptr.ciphertext_digest.is_some()); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+23
There was a problem hiding this comment. 🧹 Nitpick | 🔵 Trivial Missing negative test cases. The test suite only validates successful deserialization. Add tests for:
Do you want me to generate comprehensive negative test cases, or open an issue to track this? 🤖 Prompt for AI Agents
Comment on lines
+16
to
+23
There was a problem hiding this comment. 🛠️ Refactor suggestion | 🟠 Major Weak validation: checking Lines 20-21 map over Add format validation: #[test]
fn both_digests_allowed_when_not_low_entropy() {
let json = read_example("privacy/opaque_pointer_min.json");
let ptr: OpaquePointer = serde_json::from_str(&json).unwrap();
- let has_digest = ptr.digest.as_ref().map(|s| !s.is_empty()).unwrap_or(false);
- assert!(has_digest);
+ assert!(
+ ptr.digest.as_ref().map(|s| s.starts_with("blake3:") && s.len() == 71).unwrap_or(false),
+ "non-low-entropy pointer should have valid blake3 digest (blake3: + 64 hex chars)"
+ );
assert!(ptr.ciphertext_digest.is_some());
+ assert!(
+ ptr.ciphertext_digest.as_ref().map(|s| s.starts_with("blake3:") && s.len() == 71).unwrap_or(false),
+ "ciphertext_digest should be valid blake3 format"
+ );
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| use gatos_privacy::{OpaquePointer, VerifiedOpaquePointer}; | ||
|
|
||
| fn read_example(rel: &str) -> String { | ||
| let dir = env!("CARGO_MANIFEST_DIR"); | ||
| std::fs::read_to_string(format!("{}/../../examples/v1/{}", dir, rel)).unwrap() | ||
| } | ||
|
|
||
| #[test] | ||
| fn verified_accepts_ciphertext_only_low_entropy() { | ||
| let json = read_example("privacy/pointer_low_entropy_min.json"); | ||
| let v: VerifiedOpaquePointer = serde_json::from_str(&json).expect("verified deserialize"); | ||
| assert!(v.ciphertext_digest.is_some()); | ||
| assert!(v.digest.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn verified_rejects_low_entropy_with_plain_digest() { | ||
| let json = read_example("privacy/pointer_low_entropy_invalid.json"); | ||
| let v: Result<VerifiedOpaquePointer, _> = serde_json::from_str(&json); | ||
| assert!(v.is_err(), "should reject invalid low-entropy pointer"); | ||
| } | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate
schema-negativetarget breaks Makefile correctness.Lines 73–81 and 89–98 both define
schema-negative, causing the second definition to override the first. Make will only execute lines 89–98. The first block validates checkpoint and pointer constraints; the second validates TTL duration. Both are valid test suites—merge or rename to consolidate.Suggested fix: Consolidate both blocks into a single
schema-negativetarget:🤖 Prompt for AI Agents