Skip to content

Commit 8db59ae

Browse files
senamakelmedullabot
andcommitted
fix: address second-round review feedback on tinytools PR #1
- permission/test.rs: pin the exact serialized wire string for each PermissionLevel (in both directions), not just a round-trip, so a future serde rename fails the test instead of silently changing what gets persisted in a transcript or RPC payload - naming/types.rs: a zero (or otherwise degenerate) max_chars truncated a genuinely present value down to an empty string and still returned Some(""); treat an empty render the same as no detail and return None, with a regression test - README.md, lib.rs: replace the panicking args["text"] index in the Tool example with a safe get().and_then() chain, so the example doesn't train tool authors toward indexing into untrusted JSON Co-authored-by: Medulla <medulla@tinyhumans.ai>
1 parent c64e9bd commit 8db59ae

5 files changed

Lines changed: 52 additions & 5 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ impl Tool for Echo {
2020
})
2121
}
2222
async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
23-
Ok(ToolResult::success(args["text"].as_str().unwrap_or_default()))
23+
let text = args.get("text").and_then(|v| v.as_str()).unwrap_or("");
24+
Ok(ToolResult::success(text))
2425
}
2526
}
2627
```

‎crates/tinytools/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@
7575
//! }
7676
//!
7777
//! async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
78-
//! let text = args["text"].as_str().unwrap_or_default();
78+
//! let text = args.get("text").and_then(|v| v.as_str()).unwrap_or("");
7979
//! Ok(ToolResult::success(text))
8080
//! }
8181
//! }

‎crates/tinytools/src/naming/test.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,18 @@ fn an_ellipsis_longer_than_the_cap_cannot_overflow_it() {
114114
assert_eq!(detail.chars().count(), 2);
115115
}
116116

117+
#[test]
118+
fn a_zero_cap_yields_no_detail_rather_than_a_blank_one() {
119+
// A degenerate `max_chars` of 0 truncates a genuinely present value down to
120+
// nothing; that must read as "no detail", not as "a detail, and it's
121+
// empty" (`Some("")`), which a caller could render as a hollow decoration.
122+
let detail = context_detail_from_args_with(
123+
&json!({ "query": "hello" }),
124+
ContextDetailOptions::new(0, "..."),
125+
);
126+
assert_eq!(detail, None);
127+
}
128+
117129
#[test]
118130
fn the_default_options_are_eighty_chars_and_three_dots() {
119131
let defaults = ContextDetailOptions::default();

‎crates/tinytools/src/naming/types.rs‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -149,16 +149,27 @@ fn render_context_value(value: &Value, options: ContextDetailOptions) -> Option<
149149
if raw.is_empty() {
150150
return None;
151151
}
152-
if raw.chars().count() > options.max_chars {
152+
let rendered = if raw.chars().count() > options.max_chars {
153153
// Clamp the ellipsis itself to `max_chars` first: an ellipsis longer
154154
// than the cap (a misconfigured caller) would otherwise survive
155155
// `saturating_sub`'s zero and still be appended in full, pushing the
156156
// rendered value past the cap it was supposed to enforce.
157157
let ellipsis: String = options.ellipsis.chars().take(options.max_chars).collect();
158158
let keep = options.max_chars.saturating_sub(ellipsis.chars().count());
159159
let truncated: String = raw.chars().take(keep).collect();
160-
Some(format!("{truncated}{ellipsis}"))
160+
format!("{truncated}{ellipsis}")
161161
} else {
162-
Some(raw)
162+
raw
163+
};
164+
165+
// A zero (or otherwise degenerate) `max_chars` can truncate a genuinely
166+
// present, nonempty value down to nothing. Callers distinguish `None` (no
167+
// detail to show) from `Some(String)` (a detail to render), so surfacing
168+
// `Some("")` here would read as "there is a detail, and it's blank" rather
169+
// than "there is no detail" — treat an empty render the same as absence.
170+
if rendered.is_empty() {
171+
None
172+
} else {
173+
Some(rendered)
163174
}
164175
}

‎crates/tinytools/src/permission/test.rs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,26 @@ fn levels_round_trip_through_json() {
4040
assert_eq!(back, level);
4141
}
4242
}
43+
44+
#[test]
45+
fn levels_are_pinned_to_their_exact_wire_names() {
46+
// A round-trip alone only proves the encoder and decoder currently agree
47+
// with *each other* — it still passes if a `serde` rename quietly changes
48+
// what gets persisted. Pin the literal strings, in both directions, so a
49+
// rename that would silently corrupt an on-disk transcript or an RPC
50+
// payload already carrying `"ReadOnly"` fails here instead.
51+
let cases = [
52+
(PermissionLevel::None, "\"None\""),
53+
(PermissionLevel::ReadOnly, "\"ReadOnly\""),
54+
(PermissionLevel::Write, "\"Write\""),
55+
(PermissionLevel::Execute, "\"Execute\""),
56+
(PermissionLevel::Dangerous, "\"Dangerous\""),
57+
];
58+
for (level, wire) in cases {
59+
assert_eq!(serde_json::to_string(&level).expect("serializable"), wire);
60+
assert_eq!(
61+
serde_json::from_str::<PermissionLevel>(wire).expect("deserializable"),
62+
level
63+
);
64+
}
65+
}

0 commit comments

Comments
 (0)