Skip to content

Commit 67abef0

Browse files
ilanhilanh
authored
fix(lint): suggest one egress directive per host; fix invalid skill example (#22)
## What and why `generate` rejects a comma-separated multi-host `egress` line, but the AI skill example and the `lint` suggestions used one. - `lint` now suggests one `@openshell egress` directive per host, both in the multi-line help block (each on its own copy-pasteable line, headed "one directive per host") and in the compact one-line form (separated by `;`). - `skills/annotate-openshell-policy/SKILL.md`: the pip example is now two valid directives, and rule 2 states that there is no multi-host form. - New tests in `tests/egress_suggestions.rs`: the suggested lines are one per host and parse, and every fenced ts/python/go/sh block in SKILL.md must scan without errors, so the skill cannot drift from the grammar again. I did not add comma-separated host support to the grammar; one directive per host keeps each rule's reason and binary explicit. Independent of the other PRs. Tests live in a new file, so there are no merge conflicts with PRs that edit `tests/cli.rs`. `TOOLCHAIN=1.94 scripts/ci-local.sh` and stable clippy pass. Closes #8 Signed-off-by: Ilan Harel <[email protected]> Co-authored-by: Ilan Harel <[email protected]>
1 parent 54a9d5b commit 67abef0

3 files changed

Lines changed: 159 additions & 17 deletions

File tree

‎skills/annotate-openshell-policy/SKILL.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@ Rules of thumb (least privilege):
7272
2. Literal host and port, e.g. `api.openai.com:443`. Add `access=read-only` for read-only APIs, or
7373
`allow="POST:/v1/chat/completions"` rules when the API surface is small. Use `access=full` only when
7474
needed and say why. Avoid wildcards; if unavoidable use `*.example.com` (>= 3 labels, never `*.com`).
75+
One host per directive: `host:443` or `host:443,8443` (ports only). There is no comma-separated host list, so
76+
`pypi.org:443,files.pythonhosted.org:443` is an error: write one `egress` line per host.
7577
3. Binary: rely on `[languages]` for the usual interpreter. Use `bin=` when a subprocess makes the call
7678
(`bin=/usr/bin/git`), or a `@openshell bin` line for a whole file. Never invent a path: confirm it in the image.
7779
4. User-configurable or dynamic endpoints (`baseUrl` from settings): annotate the **default/documented**
@@ -95,7 +97,8 @@ Examples (same grammar in every language):
9597
const res = await fetch("https://api.anthropic.com/v1/messages", { ... });
9698
```
9799
```python
98-
# @openshell egress pypi.org:443,files.pythonhosted.org:443 reason="pip install at start" access=read-only
100+
# @openshell egress pypi.org:443 reason="pip install at start" access=read-only
101+
# @openshell egress files.pythonhosted.org:443 reason="pip install at start (wheel downloads)" access=read-only
99102
# @openshell fs write /workspace/cache reason="model cache"
100103
```
101104
```go

‎src/lint.rs‎

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,15 @@ impl Finding {
9797
}
9898
}
9999

100+
/// `@openshell egress` takes one `host:port[,port]`, so several hosts need one directive each.
101+
fn egress_suggestion<S: AsRef<str>>(hosts: &[S]) -> String {
102+
hosts
103+
.iter()
104+
.map(|h| format!("@openshell egress {} reason=\"...\"", h.as_ref()))
105+
.collect::<Vec<_>>()
106+
.join("; ")
107+
}
108+
100109
/// Comment syntax + window used to print copy-pasteable directives.
101110
struct Hints {
102111
leader: &'static str,
@@ -116,6 +125,14 @@ impl Hints {
116125
fn dir(&self, d: &str) -> String {
117126
format!("{} @openshell {d}", self.leader)
118127
}
128+
/// One copy-pasteable `egress` directive per host (the grammar has no multi-host form).
129+
fn egress_dirs<S: AsRef<str>>(&self, hosts: &[S]) -> String {
130+
hosts
131+
.iter()
132+
.map(|h| self.dir(&format!("egress {} reason=\"...\"", h.as_ref())))
133+
.collect::<Vec<_>>()
134+
.join("\n")
135+
}
119136
fn ignore(&self, kind: &str) -> String {
120137
self.dir(&format!("ignore kind={kind} reason=\"...\""))
121138
}
@@ -135,8 +152,13 @@ impl Hints {
135152
} else {
136153
let list = hosts.join(", ");
137154
let help = format!(
138-
"declare the destination on the line above:\n{}",
139-
self.dir(&format!("egress {list} reason=\"...\""))
155+
"declare the destination on the line above{}:\n{}",
156+
if hosts.len() > 1 {
157+
" (one directive per host)"
158+
} else {
159+
""
160+
},
161+
self.egress_dirs(hosts)
140162
);
141163
f.say(
142164
format!("unannotated network call to {list}"),
@@ -161,7 +183,7 @@ impl Hints {
161183
"this URL is not covered by any @openshell egress".into(),
162184
Some(format!(
163185
"if the program connects there, declare it:\n{}",
164-
self.dir(&format!("egress {} reason=\"...\"", all.join(", ")))
186+
self.egress_dirs(all)
165187
)),
166188
None,
167189
)
@@ -588,10 +610,7 @@ pub fn lint(scan: &Scan, cfg: &Config) -> Vec<Finding> {
588610
if hosts.is_empty() {
589611
" (no literal URL on this line: declare the host with @openshell egress, or @openshell ignore kind=net)".to_string()
590612
} else {
591-
format!(
592-
" (suggest: @openshell egress {} reason=\"...\")",
593-
hosts.join(", ")
594-
)
613+
format!(" (suggest: {})", egress_suggestion(&hosts))
595614
}
596615
} else {
597616
" (add `@openshell fs write <absolute in-sandbox path> reason=\"...\"`, or `@openshell ignore kind=fs`)".to_string()
@@ -742,14 +761,7 @@ fn lint_sites(
742761
let hint = if unknown.is_empty() {
743762
" (no literal URL in the arguments: declare the host with @openshell egress, or @openshell ignore kind=net)".to_string()
744763
} else {
745-
format!(
746-
" (suggest: @openshell egress {} reason=\"...\")",
747-
unknown
748-
.iter()
749-
.map(|x| x.as_str())
750-
.collect::<Vec<_>>()
751-
.join(", ")
752-
)
764+
format!(" (suggest: {})", egress_suggestion(&unknown))
753765
};
754766
net_lines.insert(s.line);
755767
out.push(h.net_call(
@@ -765,7 +777,16 @@ fn lint_sites(
765777
} else if !unknown.is_empty() && !net_lines.contains(&s.line) {
766778
net_lines.insert(s.line);
767779
out.push(h.url_literal(
768-
mk(s, Severity::Info, "net", format!("URL literal to {} has no matching @openshell egress (suggest: @openshell egress {} reason=\"...\")", unknown[0], unknown.iter().map(|x| x.as_str()).collect::<Vec<_>>().join(", "))),
780+
mk(
781+
s,
782+
Severity::Info,
783+
"net",
784+
format!(
785+
"URL literal to {} has no matching @openshell egress (suggest: {})",
786+
unknown[0],
787+
egress_suggestion(&unknown)
788+
),
789+
),
769790
unknown[0],
770791
&hosts,
771792
));

‎tests/egress_suggestions.rs‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
//! Lint egress suggestions are valid syntax; the AI skill examples parse.
2+
use std::path::{Path, PathBuf};
3+
use std::process::{Command, Output};
4+
5+
fn bin() -> PathBuf {
6+
PathBuf::from(env!("CARGO_BIN_EXE_openshell-annotate"))
7+
}
8+
9+
fn run(dir: &Path, args: &[&str]) -> Output {
10+
Command::new(bin())
11+
.current_dir(dir)
12+
.args(args)
13+
.output()
14+
.unwrap()
15+
}
16+
17+
fn out(o: &Output) -> String {
18+
String::from_utf8_lossy(&o.stdout).into_owned()
19+
}
20+
fn err(o: &Output) -> String {
21+
String::from_utf8_lossy(&o.stderr).into_owned()
22+
}
23+
24+
fn write(dir: &Path, rel: &str, body: &str) {
25+
let p = dir.join(rel);
26+
std::fs::create_dir_all(p.parent().unwrap()).unwrap();
27+
std::fs::write(p, body).unwrap();
28+
}
29+
30+
#[test]
31+
fn lint_suggests_one_directive_per_host_and_they_parse() {
32+
let t = tempfile::tempdir().unwrap();
33+
write(
34+
t.path(),
35+
"a.py",
36+
"import requests\nrequests.get(\"https://alpha.example.com/a?next=https://beta.example.com/b\")\n",
37+
);
38+
let c = run(t.path(), &["lint", "--format", "compact"]);
39+
let c = out(&c) + &err(&c);
40+
assert!(
41+
c.contains("(suggest: @openshell egress alpha.example.com:443 reason=\"...\"; @openshell egress beta.example.com:443 reason=\"...\")"),
42+
"{c}"
43+
);
44+
assert!(!c.contains("443, beta"), "{c}");
45+
let h = run(t.path(), &["lint", "--color", "never", "--no-icons"]);
46+
let h = out(&h) + &err(&h);
47+
assert!(h.contains("one directive per host"), "{h}");
48+
assert!(
49+
h.contains("# @openshell egress alpha.example.com:443 reason=\"...\"\n"),
50+
"{h}"
51+
);
52+
assert!(
53+
h.contains("# @openshell egress beta.example.com:443 reason=\"...\"\n"),
54+
"{h}"
55+
);
56+
57+
// the suggested lines are valid directives
58+
write(
59+
t.path(),
60+
"b.py",
61+
"# @openshell egress alpha.example.com:443 reason=\"x\" bin=/usr/bin/python3\n# @openshell egress beta.example.com:443 reason=\"x\" bin=/usr/bin/python3\n",
62+
);
63+
let g = run(t.path(), &["scan", "--format", "compact", "b.py"]);
64+
assert!(g.status.success(), "{}", err(&g));
65+
}
66+
67+
#[test]
68+
fn every_directive_in_the_skill_examples_parses() {
69+
// Fenced ts/python/go/sh blocks in the AI skill must be valid directives, so the skill cannot drift from the grammar.
70+
let md = std::fs::read_to_string(
71+
Path::new(env!("CARGO_MANIFEST_DIR")).join("skills/annotate-openshell-policy/SKILL.md"),
72+
)
73+
.unwrap();
74+
let t = tempfile::tempdir().unwrap();
75+
let mut n = 0;
76+
let mut cur: Option<(&str, String)> = None;
77+
for line in md.lines() {
78+
if let Some(lang) = line.strip_prefix("```") {
79+
match cur.take() {
80+
Some((ext, body)) => {
81+
write(t.path(), &format!("ex{n}.{ext}"), &body);
82+
n += 1;
83+
}
84+
None => {
85+
let ext = match lang.trim() {
86+
"ts" => "ts",
87+
"python" => "py",
88+
"go" => "go",
89+
"sh" => "sh",
90+
_ => "",
91+
};
92+
if !ext.is_empty() {
93+
cur = Some((ext, String::new()));
94+
} else {
95+
cur = None;
96+
}
97+
}
98+
}
99+
continue;
100+
}
101+
if let Some((_, body)) = cur.as_mut() {
102+
body.push_str(line);
103+
body.push('\n');
104+
}
105+
}
106+
assert!(n >= 4, "expected the skill's example blocks, found {n}");
107+
let o = run(t.path(), &["scan", "--format", "compact"]);
108+
assert!(
109+
o.status.success(),
110+
"skill example does not parse: {}",
111+
err(&o)
112+
);
113+
assert!(
114+
out(&o).contains("files.pythonhosted.org:443"),
115+
"{}",
116+
out(&o)
117+
);
118+
}

0 commit comments

Comments
 (0)