diff --git a/docs/reference/tools.md b/docs/reference/tools.md index 78fb2e898..9d862b644 100644 --- a/docs/reference/tools.md +++ b/docs/reference/tools.md @@ -226,7 +226,7 @@ No input properties. **Core:** Yes -
Converts natural language time to ISO datetime (e.g. 'in 2 hours', 'tomorrow at 9am', 'next Friday at 3pm'). Uses bot timezone. For schedule_task's run_at parameter.
+
Converts natural language time to ISO datetime (e.g. 'in 2 hours', 'tomorrow at 9am', 'next Friday at 3pm'). Uses bot timezone unless the expression ends in a recognized timezone. IANA zones (for example, America/New_York), EST/ET, and New York time are supported; ambiguous or unknown explicit zones are rejected. For schedule_task's run_at parameter.

[affordances: cost=free risk=none latency=instant]

diff --git a/src/discord/native_tools/media.py b/src/discord/native_tools/media.py index 4892cc368..495908ce2 100644 --- a/src/discord/native_tools/media.py +++ b/src/discord/native_tools/media.py @@ -12,6 +12,7 @@ import io import os from collections.abc import Callable +from urllib.parse import urlsplit import discord @@ -21,6 +22,25 @@ # Hard cap on a URL-fetched image so a huge or hostile body can't exhaust memory. _ANALYZE_IMAGE_MAX_BYTES = 25 * 1024 * 1024 # 25 MiB +_DISCORD_ATTACHMENT_HOSTS = frozenset({"cdn.discordapp.com", "media.discordapp.net"}) + + +def _safe_discord_attachment_url(value: object) -> str | None: + """Return a URL only when it points at Discord's HTTPS attachment CDN.""" + if not isinstance(value, str) or len(value) > 4096: + return None + try: + parsed = urlsplit(value) + if ( + parsed.scheme != "https" + or parsed.hostname not in _DISCORD_ATTACHMENT_HOSTS + or parsed.username is not None + or parsed.password is not None + ): + return None + return value + except (TypeError, ValueError): + return None class MediaTools: @@ -313,7 +333,7 @@ async def _handle_generate_image(self, message, inp: dict): } try: file = discord.File(io.BytesIO(result.data), filename="generated.png") - await message.channel.send(file=file) + posted_message = await message.channel.send(file=file) except discord.HTTPException as e: # Generation succeeded even though delivery failed — record both. meta["delivery_status"] = "upload_failed" @@ -325,6 +345,16 @@ async def _handle_generate_image(self, message, inp: dict): ) meta["delivery_status"] = "posted" + attachment_url = None + attachments = getattr(posted_message, "attachments", ()) + if attachments: + attachment_url = _safe_discord_attachment_url( + getattr(attachments[0], "url", None) + ) + if attachment_url: + meta["attachment_url_available"] = True + else: + meta["attachment_url_available"] = False log.info( "image generated: backend=%s model=%s decoded=%dx%d route=%s", result.backend, result.image_model, result.width, result.height, result.route, @@ -333,6 +363,7 @@ async def _handle_generate_image(self, message, inp: dict): output=( f"Image generated ({result.width}x{result.height}, " f"{len(result.data) / 1024:.1f} KB) and posted." + + (f" Attachment URL: {attachment_url}" if attachment_url else "") ), tool_name="generate_image", audit_metadata=meta, diff --git a/src/tools/defs/media_scheduling.py b/src/tools/defs/media_scheduling.py index 371dfbfac..c5178d229 100644 --- a/src/tools/defs/media_scheduling.py +++ b/src/tools/defs/media_scheduling.py @@ -362,7 +362,10 @@ "description": ( "Converts natural language time to ISO datetime " "(e.g. 'in 2 hours', 'tomorrow at 9am', 'next Friday at 3pm'). " - "Uses bot timezone. For schedule_task's run_at parameter." + "Uses bot timezone unless the expression ends in a recognized timezone. " + "IANA zones (for example, America/New_York), EST/ET, and New York time " + "are supported; ambiguous or unknown explicit zones are rejected. " + "For schedule_task's run_at parameter." ), "input_schema": { "type": "object", diff --git a/src/tools/post_validation.py b/src/tools/post_validation.py index ab0b2205d..51fab21bb 100644 --- a/src/tools/post_validation.py +++ b/src/tools/post_validation.py @@ -310,6 +310,17 @@ def _default_compare_for(check_type: str) -> str: # Validate the ERE first, then check journal visibility without -q; the data # run uses -q to keep journalctl's own status lines out of the matches. +# Use cat output for the data run: unlike the default journalctl rendering it +# has no host/unit prefix, so the anchored filter identifies only Odin's own +# logger and tool-call message, not unrelated logs quoting the same text. Filter +# before the caller's regex, rather than trying to clean the matched output: +# a match only in the invocation must not prove presence or disprove absence. +_VALIDATION_INVOCATION_LINE = ( + # Python logging's default asctime includes comma-separated milliseconds; + # accept older second-resolution journal entries as well. + r"^[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}(,[0-9]{3})? " + r"\[INFO\] odin\.discord: Tool call: validate_action\(\{" +) _LOG_PROBE_SCRIPT = ( 'p="$3"; ' 'printf "" | grep -E -e "$p" >/dev/null 2>&1; ' @@ -320,7 +331,9 @@ def _default_compare_for(check_type: str) -> str: 'e=$(journalctl "$@" --no-pager -n 1 2>&1 >/dev/null); r=$?; ' 'if [ "$r" -ne 0 ]; then echo "LOG_CHECK_ERROR journalctl exit $r"; ' 'printf "%s\\n" "$e" | tail -n 1; exit 0; fi; ' - 'journalctl "$@" --no-pager -q 2>/dev/null | grep -E -e "$p" | head -n 20; ' + 'journalctl "$@" --no-pager -q -o cat 2>/dev/null | ' + f'grep -v -E -e {shlex.quote(_VALIDATION_INVOCATION_LINE)} | ' + 'grep -E -e "$p" | head -n 20; ' 'case "$e" in *"not seeing messages from"*|*"insufficient permissions"*' '|*"No journal files were found"*) echo LOG_READ_PARTIAL;; *) echo LOG_READ_OK;; esac' ) diff --git a/src/tools/time_parser.py b/src/tools/time_parser.py index df4a4f515..db7af8c02 100644 --- a/src/tools/time_parser.py +++ b/src/tools/time_parser.py @@ -83,6 +83,69 @@ def set_default_timezone(tz_name: str) -> None: + r")\b|may\s+\d)" ) +# Abbreviations are not reliable identifiers for time zones. EST/ET are +# explicitly treated as the colloquial US Eastern zone (including DST); other +# common abbreviations are rejected rather than guessed or silently ignored. +_ZONE_ALIASES = {"est": "America/New_York", "et": "America/New_York"} +_ZONE_ALIASES.update({"utc": "UTC", "gmt": "UTC"}) +_AMBIGUOUS_ZONE_ABBREVIATIONS = { + "ast", "bst", "cst", "ist", "mst", "pst", "adt", "cdt", "edt", "mdt", "pdt" +} +_IANA_ZONE_SUFFIX = re.compile( + r"(?:\s+in)?\s+([a-z_+-]+(?:/[a-z0-9_+.-]+)+)$", re.IGNORECASE +) +_NAMED_ZONE_SUFFIX = re.compile( + r"\s+(?:in\s+)?(new\s+york|eastern)(?:\s+time)?$", re.IGNORECASE +) +_ABBREVIATION_SUFFIX = re.compile(r"\s+(?:in\s+)?([a-z]{2,5})$", re.IGNORECASE) +_EXPLICIT_ZONE_PHRASE = re.compile(r"\s+in\s+([a-z][a-z0-9_+./ -]*)$", re.IGNORECASE) + + +def _extract_explicit_timezone(expression: str) -> tuple[str, ZoneInfo | None]: + """Remove a recognized trailing zone, rejecting explicit but unsafe zones.""" + text = expression.strip() + match = _IANA_ZONE_SUFFIX.search(text) + if match: + zone_name = match.group(1) + try: + zone = ZoneInfo(zone_name) + except (KeyError, ValueError) as exc: + raise ValueError(f"Unknown timezone: {zone_name}") from exc + return text[: match.start()].strip(), zone + + match = _NAMED_ZONE_SUFFIX.search(text) + if match: + return text[: match.start()].strip(), ZoneInfo("America/New_York") + + match = _ABBREVIATION_SUFFIX.search(text) + if match: + abbreviation = match.group(1).lower() + if abbreviation in _ZONE_ALIASES: + return text[: match.start()].strip(), ZoneInfo(_ZONE_ALIASES[abbreviation]) + # Only uppercase source tokens are presumed to be explicit abbreviations; + # ordinary prose tails retain the parser's historical behavior. + source_token = text[match.start(1) : match.end(1)] + if source_token.isupper() or abbreviation in _AMBIGUOUS_ZONE_ABBREVIATIONS: + raise ValueError( + f"Timezone abbreviation '{source_token}' is ambiguous or unsupported; " + "use an IANA zone such as America/New_York" + ) + + # An explicit "in " clause is not harmless trailing prose. If it + # was not one of the supported aliases or a valid IANA identifier, fail + # closed instead of silently scheduling in the configured default zone. + # Conventional time-of-day prose isn't a zone request: let the clock + # parser give its normal actionable time-format error instead. + if re.search(r"\s+in\s+the\s+(?:morning|afternoon|evening|night)$", text, re.IGNORECASE): + return text, None + match = _EXPLICIT_ZONE_PHRASE.search(text) + if match: + raise ValueError( + f"Unrecognized timezone '{match.group(1).strip()}'; use an IANA zone " + "such as America/New_York" + ) + return text, None + def _split_time_of_day(text: str) -> tuple[tuple[int, int], str] | None: """Return a leading clock time and the unconsumed text.""" @@ -168,10 +231,14 @@ def _next_weekday(now: datetime, target_weekday: int) -> datetime: def parse_time(expression: str, now: datetime | None = None) -> str: """Parse a natural language time expression into an ISO datetime string.""" + expression, explicit_tz = _extract_explicit_timezone(expression) + tz = explicit_tz or _default_tz if now is None: - now = datetime.now(_default_tz) + now = datetime.now(tz) elif now.tzinfo is None: - now = now.replace(tzinfo=_default_tz) + now = now.replace(tzinfo=tz) + elif explicit_tz is not None: + now = now.astimezone(explicit_tz) text = expression.strip().lower() m = re.match(r"in\s+(\d+)\s+(\w+)", text) diff --git a/tests/characterization/test_tool_parity.py b/tests/characterization/test_tool_parity.py index 487e57f0b..fae43aa1a 100644 --- a/tests/characterization/test_tool_parity.py +++ b/tests/characterization/test_tool_parity.py @@ -68,7 +68,8 @@ # Updated with schedule_task: shared trigger shape, report_format clearing (B2). "update_schedule": "ba5b0ca8c5b1f96a", "delete_schedule": "01e54d37b70471a8", - "parse_time": "6ae3f4c04138a2cd", + # Explicit-zone support changes the user-facing description, not input shape. + "parse_time": "b732643e002f9fad", "search_history": "72aaa6b1024b0fc0", "memory_manage": "f7aa460db948c1d5", "search_audit": "6fcb11f91a34bcb6", diff --git a/tests/test_campaign_a_validation.py b/tests/test_campaign_a_validation.py index 7e797f167..7682716b5 100644 --- a/tests/test_campaign_a_validation.py +++ b/tests/test_campaign_a_validation.py @@ -358,6 +358,58 @@ def test_readable_journal_still_matches(tmp_path, ctype, expected): assert _shell_check(Check(type=ctype, target="FIXTUREERROR"), env=env)[0] == expected +@pytest.mark.parametrize("unit", ["", "unit=odin:"]) +@pytest.mark.parametrize("pattern", ["BATTERY-NONEXISTENT-9f2c", "Startup diagnostics"]) +def test_log_probe_ignores_own_invocation_for_absent_and_present(tmp_path, unit, pattern): + # Actual journalctl -u odin -q -o cat output uses comma milliseconds: + # 2026-09-27 23:21:56,515 [INFO] odin.discord: Tool call: validate_action({'checks': ... + # Without filtering, it proves presence and disproves absence. + invocation = ( + "2026-09-27 23:21:56,515 [INFO] odin.discord: " + "Tool call: validate_action({'checks': [{'type': 'log_absent', " + f"'target': 'unit=odin:{pattern}'}}]}})" + ) + env = _fake_journal(tmp_path, body=invocation) + assert _shell_check(Check(type="log_absent", target=f"{unit}{pattern}"), env=env)[0] == "pass" + assert _shell_check(Check(type="log_present", target=f"{unit}{pattern}"), env=env)[0] == "fail" + + +@pytest.mark.parametrize("unit", ["", "unit=odin:"]) +def test_log_probe_preserves_genuine_messages_even_with_invocation_text(tmp_path, unit): + invocation = ( + "2026-09-28 12:00:00 [INFO] odin.discord: " + "Tool call: validate_action({'target': 'unit=odin:Startup diagnostics'})" + ) + real_message = "2026-09-28 12:00:01 [INFO] odin.discord: Startup diagnostics completed" + quoted_message = ( + "2026-09-28 12:00:02 [INFO] odin.service: " + "User wrote Tool call: validate_action(Startup diagnostics)" + ) + env = _fake_journal(tmp_path, body="\n".join((invocation, real_message, quoted_message))) + check = Check(type="log_present", target=f"{unit}Startup diagnostics") + assert _shell_check(check, env=env)[0] == "pass" + check = Check(type="log_absent", target=f"{unit}Startup diagnostics") + assert _shell_check(check, env=env)[0] == "fail" + + # Quoting the invocation is still a real log message, not an Odin call. + env = _fake_journal(tmp_path, body=quoted_message) + assert _shell_check(check, env=env)[0] == "fail" + + +def test_log_probe_filters_before_limiting_matches(tmp_path): + invocations = "\n".join( + "2026-09-28 12:00:00,123 [INFO] odin.discord: " + f"Tool call: validate_action({{'target': 'unit=odin:Startup diagnostics', " + f"'attempt': {i}}})" + for i in range(25) + ) + env = _fake_journal(tmp_path, body=f"{invocations}\nStartup diagnostics completed") + check = Check(type="log_present", target="Startup diagnostics") + assert _shell_check(check, env=env)[0] == "pass" + check = Check(type="log_absent", target="Startup diagnostics") + assert _shell_check(check, env=env)[0] == "fail" + + @pytest.mark.parametrize( "ctype,output,expected", [ diff --git a/tests/test_campaign_discord_final_coverage.py b/tests/test_campaign_discord_final_coverage.py index 01c411250..be806efde 100644 --- a/tests/test_campaign_discord_final_coverage.py +++ b/tests/test_campaign_discord_final_coverage.py @@ -216,8 +216,9 @@ async def start_mcp_stub(received): @pytest.mark.asyncio -async def test_application_startup_tolerates_quota_check_failure(monkeypatch): - """The optional quota poller is isolated from required app startup.""" +@pytest.mark.parametrize("diagnostics_fail", [False, True]) +async def test_application_startup_tolerates_quota_check_failure(monkeypatch, diagnostics_fail): + """Optional quota and diagnostic failures cannot prevent required startup.""" import src.discord.client as client_module bot = object.__new__(OdinBot) @@ -235,7 +236,13 @@ async def test_application_startup_tolerates_quota_check_failure(monkeypatch): ) bot.load_extension = AsyncMock() bot.computer = SimpleNamespace(start=AsyncMock()) - bot._run_startup_diagnostics = lambda *, yaml_config: SimpleNamespace(results=[]) + + def diagnostics(*, yaml_config): + if diagnostics_fail: + raise RuntimeError("diagnostics unavailable") + return SimpleNamespace(results=[]) + + bot._run_startup_diagnostics = diagnostics monkeypatch.setattr(client_module, "INITIAL_EXTENSIONS", ()) monkeypatch.setattr(client_module, "start_mcp", AsyncMock()) @@ -243,6 +250,8 @@ async def test_application_startup_tolerates_quota_check_failure(monkeypatch): bot.codex_quota_check.start.assert_awaited_once() assert bot._application_started is True + if diagnostics_fail: + assert not hasattr(bot, "startup_report") def test_startup_config_logging_covers_nonempty_host_and_option_flags(): diff --git a/tests/test_native_media.py b/tests/test_native_media.py index 505f3c8be..202525d5c 100644 --- a/tests/test_native_media.py +++ b/tests/test_native_media.py @@ -12,12 +12,34 @@ from unittest.mock import AsyncMock, MagicMock, patch import discord -from src.discord.native_tools.media import MediaTools +from src.discord.native_tools.media import MediaTools, _safe_discord_attachment_url from src.tools.hosts import HostRegistry PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32 +class TestSafeDiscordAttachmentUrl: + def test_accepts_only_https_discord_attachment_hosts(self): + url = "https://cdn.discordapp.com/attachments/1/2/image.png?ex=abc" + assert _safe_discord_attachment_url(url) == url + assert _safe_discord_attachment_url( + "https://media.discordapp.net/attachments/1/2/image.png" + ) == "https://media.discordapp.net/attachments/1/2/image.png" + + def test_rejects_invalid_values_and_unsafe_urls(self): + assert _safe_discord_attachment_url(None) is None + assert _safe_discord_attachment_url("x" * 4097) is None + for url in ( + "http://cdn.discordapp.com/attachments/1/2/image.png", + "https://discordapp.com/attachments/1/2/image.png", + "https://cdn.discordapp.com.evil.example/image.png", + "https://user@cdn.discordapp.com/image.png", + "https://user:secret@cdn.discordapp.com/image.png", + "https://[invalid-ipv6/image.png", + ): + assert _safe_discord_attachment_url(url) is None + + def _http_exc(status=500): return discord.HTTPException( SimpleNamespace(status=status, reason="err"), "msg") # type: ignore[arg-type] @@ -349,6 +371,9 @@ async def test_no_selector_and_no_prompt(self): async def test_success_posts_attachment(self): sel = self._selector(result=self._result(backend="openai")) msg = _message() + msg.channel.send.return_value = SimpleNamespace(attachments=[ + SimpleNamespace(url="https://cdn.discordapp.com/attachments/123/456/generated.png?ex=abc") + ]) out = await _tools(image_selector=sel)._handle_generate_image(msg, {"prompt": "a cat"}) # Generic user-facing string — the backend name is NOT surfaced there... assert "Image generated (1024x1024" in str(out) @@ -357,6 +382,27 @@ async def test_success_posts_attachment(self): # ...but IS recorded in the (non-model-facing) audit metadata. assert out.audit_metadata["backend"] == "openai" assert out.audit_metadata["delivery_status"] == "posted" + assert "https://cdn.discordapp.com/attachments/123/456/generated.png?ex=abc" in str(out) + assert out.audit_metadata["attachment_url_available"] is True + + async def test_success_does_not_expose_non_discord_attachment_url(self): + sel = self._selector(result=self._result()) + msg = _message() + msg.channel.send.return_value = SimpleNamespace(attachments=[ + SimpleNamespace(url="https://attacker.example/image.png?token=secret") + ]) + out = await _tools(image_selector=sel)._handle_generate_image(msg, {"prompt": "x"}) + assert "attacker.example" not in str(out) + assert "token=secret" not in str(out) + assert out.audit_metadata["attachment_url_available"] is False + + async def test_attachment_url_guard_rejects_lookalikes_and_credentials(self): + from src.discord.native_tools.media import _safe_discord_attachment_url + + assert _safe_discord_attachment_url("https://cdn.discordapp.com/a.png") + assert _safe_discord_attachment_url("https://cdn.discordapp.com.evil/a.png") is None + assert _safe_discord_attachment_url("https://user@cdn.discordapp.com/a.png") is None + assert _safe_discord_attachment_url("http://cdn.discordapp.com/a.png") is None async def test_backend_failure_and_http_error(self): from src.tools.image import ImageGenError diff --git a/tests/test_post_validation.py b/tests/test_post_validation.py index dcbeaf4c7..4406a0566 100644 --- a/tests/test_post_validation.py +++ b/tests/test_post_validation.py @@ -177,6 +177,7 @@ def test_service(self): def test_log_absent_unit(self): cmd = _build_command(Check(type="log_absent", target="unit=nginx:ERROR")) assert "journalctl" in cmd + assert "-o cat" in cmd assert "grep" in cmd assert shlex.split(cmd)[-3:] == ["nginx", "120", "ERROR"] diff --git a/tests/test_time_parser.py b/tests/test_time_parser.py index 3dd1f9918..84aabb814 100644 --- a/tests/test_time_parser.py +++ b/tests/test_time_parser.py @@ -169,7 +169,7 @@ def test_rejects_unconsumed_time_words(self, expression): @pytest.mark.parametrize("tail", [ " please", " from now", " or so", " at the latest", " to check the 3 servers", - " tonight", " today", " est", " utc", + " tonight", " today", ]) def test_harmless_tails(self, tail): assert parse_time("in 2 hours" + tail, NOW) == parse_time("in 2 hours", NOW) @@ -225,3 +225,45 @@ def now(cls, tz=None): monkeypatch.setattr(time_parser, "datetime", FixedClock) result = datetime.fromisoformat(parse_time("in 2 hours")) assert result.astimezone(UTC) == datetime(2026, 3, 8, 8, 30, tzinfo=UTC) + + +class TestExplicitTimezone: + @pytest.mark.parametrize("expression", [ + "tomorrow at 9am in America/New_York", + "tomorrow at 9am EST", + "tomorrow at 9am ET", + "tomorrow at 9am New York time", + ]) + def test_explicit_new_york_zone_overrides_default_and_obeys_dst(self, expression): + now = datetime(2026, 3, 18, 12, tzinfo=UTC) + assert parse_time(expression, now) == "2026-03-19T09:00:00-04:00" + + def test_explicit_zone_converts_now_before_resolving_relative_dates(self): + # 01:00 UTC on Wednesday is still Tuesday evening in New York. + now = datetime(2026, 3, 18, 1, tzinfo=UTC) + assert parse_time("tomorrow at 9am America/New_York", now) == ( + "2026-03-18T09:00:00-04:00" + ) + + @pytest.mark.parametrize("expression", [ + "tomorrow at 9am in NotAReal/Zone", + "tomorrow at 9am in Mars Standard Time", + "tomorrow at 9am CST", + "tomorrow at 9am in CST", + "tomorrow at 9am PST", + ]) + def test_rejects_unknown_or_ambiguous_explicit_zone(self, expression): + with pytest.raises(ValueError, match="timezone|Timezone"): + parse_time(expression, NOW) + + def test_utc_alias_is_explicit(self): + assert parse_time("tomorrow at 9am UTC", NOW) == "2026-03-19T09:00:00+00:00" + + @pytest.mark.parametrize("expression", [ + "tomorrow at 9 in the morning", + "tomorrow at 3 in the afternoon", + "tomorrow at 7 in the evening", + ]) + def test_time_of_day_prose_reports_time_error_not_timezone(self, expression): + with pytest.raises(ValueError, match="^Cannot parse time:"): + parse_time(expression, NOW)