Skip to content

Commit 308cf4b

Browse files
fix(stations/notify): preserve scanner status and harden packaging
Add buildRegistry/send rejection for non-positive defaults.timeout_seconds while preserving the existing negative-timeout doctor diagnostic (only an absent or explicit zero defaults to 10). Distinguish content-guard findings from scanner failures in the notify pre-push hook (capture rc, gate findings on rc==1, preserve other nonzero statuses), make Makefile install fail closed for missing HOME, and make the package platform loop fail fast with set -eu on the same logical recipe line. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 4f52d61 commit 308cf4b

7 files changed

Lines changed: 300 additions & 1 deletion

File tree

stations/notify/Makefile

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ cross: clean
3030
GOOS=darwin GOARCH=arm64 go build $(LDFLAGS) -o dist/$(BINARY)-darwin-arm64 $(PKG)
3131

3232
install: build
33+
$(if $(strip $(HOME)),,$(error HOME is not set; refusing to install into /bin))
34+
mkdir -p $(HOME)/bin
3335
install -m 0755 $(BINARY) $(HOME)/bin/$(BINARY)
3436

3537
clean-dist:
@@ -38,6 +40,7 @@ clean-dist:
3840
# Build per-platform archives with checksums. Output to dist/.
3941
package: clean-dist
4042
mkdir -p dist/tmp
43+
set -eu; \
4144
for platform in $(PLATFORMS); do \
4245
os=$${platform%/*}; \
4346
arch=$${platform#*/}; \

stations/notify/cmd/agent-notify/main.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,9 @@ func sortedProfileNames(cfg *config.Config) []string {
332332
}
333333

334334
func buildRegistry(cfg *config.Config, names []string) (*channels.Registry, error) {
335+
if cfg.Defaults.TimeoutSeconds <= 0 {
336+
return nil, fmt.Errorf("defaults.timeout_seconds must be greater than zero, got %d", cfg.Defaults.TimeoutSeconds)
337+
}
335338
reg := channels.NewRegistry()
336339
timeout := time.Duration(cfg.Defaults.TimeoutSeconds) * time.Second
337340

stations/notify/cmd/agent-notify/main_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616

1717
"github.com/escoffier-labs/agent-notify/internal/canonical"
1818
"github.com/escoffier-labs/agent-notify/internal/channels"
19+
"github.com/escoffier-labs/agent-notify/internal/config"
1920
)
2021

2122
type leakingErrorChannel struct {
@@ -442,6 +443,99 @@ func TestRun_DoctorJSONReportsMissingConfigAsUnconfigured(t *testing.T) {
442443
}
443444
}
444445

446+
func TestBuildRegistryRejectsNonPositiveTimeout(t *testing.T) {
447+
// A non-positive timeout must never reach channel construction: the
448+
// registry build fails with a config error naming the offending key.
449+
// The webhook URL is required so the pre-fix RED state reaches channel
450+
// construction instead of failing earlier with missing credentials.
451+
t.Setenv("DISCORD_WEBHOOK_URL", "https://discord.test/webhook/123")
452+
for _, timeout := range []int{-5, 0} {
453+
cfg := &config.Config{
454+
Channels: map[string]config.ChannelConfig{
455+
"discord-main": {Type: "discord", WebhookURLEnv: "DISCORD_WEBHOOK_URL"},
456+
},
457+
Defaults: config.Defaults{TimeoutSeconds: timeout},
458+
}
459+
_, err := buildRegistry(cfg, []string{"discord-main"})
460+
if err == nil {
461+
t.Fatalf("timeout %d: expected buildRegistry error, got nil", timeout)
462+
}
463+
if !strings.Contains(err.Error(), "defaults.timeout_seconds") {
464+
t.Errorf("timeout %d: error should name defaults.timeout_seconds, got %v", timeout, err)
465+
}
466+
}
467+
}
468+
469+
func TestRun_DoctorJSONFailsOnNegativeTimeout(t *testing.T) {
470+
// doctor must surface an invalid negative timeout as a FAIL check on
471+
// defaults.timeout_seconds, not silently normalize it away.
472+
cfgPath := filepath.Join(t.TempDir(), "config.toml")
473+
if err := os.WriteFile(cfgPath, []byte(`
474+
[defaults]
475+
timeout_seconds = -5
476+
477+
[channels.discord-main]
478+
type = "discord"
479+
webhook_url_env = "DISCORD_WEBHOOK_URL"
480+
`), 0o644); err != nil {
481+
t.Fatal(err)
482+
}
483+
484+
code, stdout, stderr := runMain(t,
485+
[]string{"agent-notify", "doctor", "--json", "--config", cfgPath},
486+
"",
487+
map[string]string{"DISCORD_WEBHOOK_URL": "https://discord.test/webhook/123"},
488+
)
489+
if code != 2 {
490+
t.Fatalf("exit = %d, want 2 (stderr=%s)", code, stderr)
491+
}
492+
var payload map[string]interface{}
493+
if err := json.Unmarshal([]byte(stdout), &payload); err != nil {
494+
t.Fatalf("invalid json: %v\n%s", err, stdout)
495+
}
496+
found := false
497+
for _, c := range payload["checks"].([]interface{}) {
498+
check := c.(map[string]interface{})
499+
if check["name"] == "defaults.timeout_seconds" {
500+
found = true
501+
if check["status"] != "FAIL" {
502+
t.Fatalf("defaults.timeout_seconds status = %v, want FAIL", check["status"])
503+
}
504+
}
505+
}
506+
if !found {
507+
t.Fatalf("expected a defaults.timeout_seconds check in %#v", payload["checks"])
508+
}
509+
}
510+
511+
func TestRun_SendRejectsNegativeTimeout(t *testing.T) {
512+
// The send path must refuse a negative timeout with a config error
513+
// instead of constructing channels with a non-positive deadline.
514+
cfgPath := filepath.Join(t.TempDir(), "config.toml")
515+
if err := os.WriteFile(cfgPath, []byte(`
516+
[defaults]
517+
timeout_seconds = -5
518+
519+
[channels.discord-main]
520+
type = "discord"
521+
webhook_url_env = "DISCORD_WEBHOOK_URL"
522+
`), 0o644); err != nil {
523+
t.Fatal(err)
524+
}
525+
526+
code, _, stderr := runMain(t,
527+
[]string{"agent-notify", "--config", cfgPath, "build done"},
528+
"",
529+
map[string]string{"DISCORD_WEBHOOK_URL": "https://discord.test/webhook/123"},
530+
)
531+
if code != exitConfig {
532+
t.Fatalf("exit = %d, want %d (stderr=%s)", code, exitConfig, stderr)
533+
}
534+
if !strings.Contains(stderr, "defaults.timeout_seconds") {
535+
t.Fatalf("stderr should name defaults.timeout_seconds, got %s", stderr)
536+
}
537+
}
538+
445539
func TestRun_StatusJSONWarnsForInactiveChannelMissingEnv(t *testing.T) {
446540
cfgPath := filepath.Join(t.TempDir(), "config.toml")
447541
if err := os.WriteFile(cfgPath, []byte(`

stations/notify/hooks/pre-push

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,24 @@ fi
2121
REPO_ROOT="$(git rev-parse --show-toplevel)"
2222
echo "pre-push: scanning $REPO_ROOT against $(basename "$POLICY")"
2323

24-
if ! PYTHONPATH="$CONTENT_GUARD_DIR/src" python3 -m content_guard scan "$REPO_ROOT" --policy "$POLICY"; then
24+
rc=0
25+
PYTHONPATH="$CONTENT_GUARD_DIR/src" python3 -m content_guard scan "$REPO_ROOT" --policy "$POLICY" || rc=$?
26+
27+
if [[ "$rc" -eq 0 ]]; then
28+
exit 0
29+
fi
30+
31+
if [[ "$rc" -eq 1 ]]; then
2532
echo >&2
2633
echo "pre-push: BLOCKED. content-guard found violations." >&2
2734
echo "pre-push: fix the leak, or add an inline tag on the offending line:" >&2
2835
echo "pre-push: <!-- content-guard: allow <rule-id> -->" >&2
2936
exit 1
3037
fi
38+
39+
# Any other exit code is the scanner failing to run (missing deps, bad policy,
40+
# crash), not a leak verdict. Preserve the original status.
41+
echo >&2
42+
echo "pre-push: content-guard failed to run (exit code $rc); this is a scanner error, not a leak verdict." >&2
43+
echo "pre-push: re-run it directly to see the error, then push again once the scanner works." >&2
44+
exit "$rc"

stations/notify/internal/config/config.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ func Load(path string) (*Config, error) {
6666
return nil, fmt.Errorf("decode config: %w", err)
6767
}
6868

69+
// Zero means "not set" (absent or explicit) and falls back to the
70+
// default. A negative value is invalid configuration and stays
71+
// observable: doctor reports it as FAIL and the send path rejects it
72+
// rather than silently substituting the default.
6973
if cfg.Defaults.TimeoutSeconds == 0 {
7074
cfg.Defaults.TimeoutSeconds = 10
7175
}

stations/notify/internal/config/config_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,3 +106,46 @@ func TestLoad_DefaultTimeoutIs10s(t *testing.T) {
106106
t.Errorf("expected default timeout 10s, got %d", cfg.Defaults.TimeoutSeconds)
107107
}
108108
}
109+
110+
func TestLoad_ZeroTimeoutNormalizesTo10s(t *testing.T) {
111+
dir := t.TempDir()
112+
path := filepath.Join(dir, "config.toml")
113+
body := `
114+
[defaults]
115+
timeout_seconds = 0
116+
`
117+
if err := os.WriteFile(path, []byte(body), 0o644); err != nil {
118+
t.Fatal(err)
119+
}
120+
121+
cfg, err := Load(path)
122+
if err != nil {
123+
t.Fatalf("Load failed: %v", err)
124+
}
125+
if cfg.Defaults.TimeoutSeconds != 10 {
126+
t.Errorf("expected zero timeout normalized to 10s, got %d", cfg.Defaults.TimeoutSeconds)
127+
}
128+
}
129+
130+
func TestLoad_NegativeTimeoutStaysObservable(t *testing.T) {
131+
// A negative timeout is invalid configuration, not a request for the
132+
// default. Load must surface it so doctor can FAIL on it and the send
133+
// path can reject it, instead of silently substituting 10s.
134+
dir := t.TempDir()
135+
path := filepath.Join(dir, "config.toml")
136+
body := `
137+
[defaults]
138+
timeout_seconds = -5
139+
`
140+
if err := os.WriteFile(path, []byte(body), 0o644); err != nil {
141+
t.Fatal(err)
142+
}
143+
144+
cfg, err := Load(path)
145+
if err != nil {
146+
t.Fatalf("Load failed: %v", err)
147+
}
148+
if cfg.Defaults.TimeoutSeconds != -5 {
149+
t.Errorf("expected negative timeout preserved as -5, got %d", cfg.Defaults.TimeoutSeconds)
150+
}
151+
}
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
"""Contract checks for stations/notify packaging and pre-push hook behavior."""
2+
3+
from __future__ import annotations
4+
5+
import os
6+
import shutil
7+
import subprocess
8+
from pathlib import Path
9+
10+
import pytest
11+
12+
13+
ROOT = Path(__file__).resolve().parents[1]
14+
NOTIFY = ROOT / "stations" / "notify"
15+
16+
17+
def _makefile() -> str:
18+
return (NOTIFY / "Makefile").read_text()
19+
20+
21+
def _pre_push() -> str:
22+
return (NOTIFY / "hooks" / "pre-push").read_text()
23+
24+
25+
def _logical_shell_lines(recipe: str) -> list[str]:
26+
"""Join backslash-continued physical lines into single shell logical lines.
27+
28+
Make runs each logical recipe line in its own shell, so fail-fast flags
29+
only protect commands on the same continued line.
30+
"""
31+
logical: list[str] = []
32+
buf = ""
33+
for line in recipe.splitlines():
34+
stripped = line.rstrip()
35+
if stripped.endswith("\\"):
36+
buf += stripped[:-1] + " "
37+
else:
38+
logical.append(buf + stripped)
39+
buf = ""
40+
if buf:
41+
logical.append(buf)
42+
return logical
43+
44+
45+
def test_notify_pre_push_captures_scanner_exit_code():
46+
text = _pre_push()
47+
assert "|| rc=$?" in text
48+
assert "if ! PYTHONPATH=" not in text
49+
50+
51+
def test_notify_pre_push_blocks_findings_with_exit_1():
52+
text = _pre_push()
53+
assert '"$rc" -eq 1' in text
54+
assert "BLOCKED. content-guard found violations." in text
55+
# The exit-1 assertion is scoped to the findings branch; the pre-existing
56+
# guard clauses at the top of the hook must not satisfy it.
57+
branch_start = text.index('if [[ "$rc" -eq 1 ]]')
58+
branch_end = text.index('exit "$rc"')
59+
branch = text[branch_start:branch_end]
60+
assert "exit 1" in branch
61+
62+
63+
def test_notify_pre_push_reports_scanner_errors_and_preserves_status():
64+
text = _pre_push()
65+
assert "failed to run" in text
66+
assert "not a leak verdict" in text
67+
assert 'exit "$rc"' in text
68+
69+
70+
def test_notify_makefile_install_creates_home_bin():
71+
text = _makefile()
72+
install = text[text.index("install:") : text.index("clean-dist:")]
73+
assert "mkdir -p $(HOME)/bin" in install
74+
assert "install -m 0755 $(BINARY) $(HOME)/bin/$(BINARY)" in install
75+
assert install.index("mkdir -p $(HOME)/bin") < install.index("install -m 0755")
76+
77+
78+
def test_notify_makefile_install_fails_closed_without_home():
79+
# An unset/empty HOME must abort the recipe before mkdir -p /bin and
80+
# install into /bin can run.
81+
text = _makefile()
82+
install = text[text.index("install:") : text.index("clean-dist:")]
83+
assert "$(error" in install, "install target has no $(error ...) guard"
84+
guard = next(line for line in install.splitlines() if "$(error" in line)
85+
assert "HOME" in guard
86+
assert install.index("$(error") < install.index("mkdir -p $(HOME)/bin")
87+
88+
89+
def test_notify_makefile_install_empty_home_aborts():
90+
if shutil.which("make") is None:
91+
pytest.skip("GNU make not available")
92+
env = {**os.environ, "HOME": ""}
93+
r = subprocess.run(
94+
["make", "-n", "install"],
95+
cwd=NOTIFY,
96+
env=env,
97+
capture_output=True,
98+
text=True,
99+
)
100+
assert r.returncode != 0, f"expected make to abort, got stdout={r.stdout!r}"
101+
assert "HOME is not set; refusing to install into /bin" in r.stderr
102+
103+
104+
def test_notify_makefile_install_nonempty_home_dry_run_succeeds(tmp_path):
105+
if shutil.which("make") is None:
106+
pytest.skip("GNU make not available")
107+
home = str(tmp_path / "home")
108+
env = {**os.environ, "HOME": home}
109+
r = subprocess.run(
110+
["make", "-n", "install"],
111+
cwd=NOTIFY,
112+
env=env,
113+
capture_output=True,
114+
text=True,
115+
)
116+
assert r.returncode == 0, f"make failed: stderr={r.stderr!r}"
117+
assert "mkdir -p" in r.stdout
118+
assert "install -m 0755" in r.stdout
119+
120+
121+
def test_notify_makefile_package_fails_fast_in_platform_loop():
122+
text = _makefile()
123+
package = text[text.index("package:") :]
124+
for step in (
125+
"go build",
126+
"cp README.md LICENSE",
127+
"chmod 755",
128+
"tar -C dist/tmp",
129+
):
130+
assert step in package
131+
# set -eu must share one backslash-continued shell with the platform
132+
# loop; moved onto its own recipe line it runs in a separate shell and
133+
# the loop no longer fails fast.
134+
loop_shells = [line for line in _logical_shell_lines(package) if "for platform" in line]
135+
assert loop_shells, "platform loop not found in package recipe"
136+
for shell in loop_shells:
137+
assert "set -eu" in shell
138+
assert shell.index("set -eu") < shell.index("for platform")

0 commit comments

Comments
 (0)