Repository navigation
fix(discord-read): render timestamps in the owner's timezone, never raw UTC - #2270
liususan091219 wants to merge 13 commits into
Conversation
|
|
Coverage Gate❌ Test suite failed under instrumentation — coverage not measurable. See the job log. |
john-the-dev
left a comment
There was a problem hiding this comment.
The behavior is reasonable, but this is not merge-ready while the required diff-coverage check is red (new timezone branches currently have no automated coverage). Please extract the timestamp conversion into a testable helper and add cases for: valid OWNER_TZ, host-timezone fallback, naive timestamp treated as UTC, invalid timezone/timestamp honest UTC fallback, and an explicit timezone abbreviation in output. A live Discord test is not necessary for this pure formatting change; deterministic unit coverage is the right evidence. Also rerun/unstick CLA after the update.
|
Agreed on deterministic coverage over a live test for a pure formatting change. Will extract the conversion into a helper and add the five cases you listed (valid OWNER_TZ / host-tz fallback / naive-as-UTC / invalid-tz honest fallback / abbreviation in output), then poke CLA. Scheduled for 2026-07-24. |
qingyun-wu
left a comment
There was a problem hiding this comment.
The unresolved deterministic-test blocker remains on the current head: src/discord-read.py:104-123 changes user-visible timestamp conversion and fallback behavior without coverage. Please add cases for OWNER_TZ, host-timezone fallback, naive/invalid timestamps, invalid timezone values, and truthful UTC fallback (including the displayed abbreviation). This is needed before approving a timezone-sensitive formatter.
…aw UTC Rebased onto main's refactored discord-read.py (main()/helpers — the old branch predated it and would have reverted it). The conversion now lives in a testable format_timestamp(raw, owner_tz) helper: OWNER_TZ env > host OS timezone, %Z label always explicit, honest UTC-labeled fallback on garbage input or unknown timezone. tests/discord-read-timestamps.test.py covers the reviewer-requested matrix (valid OWNER_TZ summer/winter, host-tz fallback via TZ+tzset, naive-treated-as-UTC, invalid tz, invalid timestamp, explicit abbreviation) plus a mocked end-to-end main() run. Diff coverage 100%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d253870 to
f874a28
Compare
|
Rebased onto current main (the branch predated the discord-read.py main()/helpers refactor and would have reverted it) and addressed both reviews: the conversion is extracted into a testable |
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f874a282e9faf17d107197c695f0ab3aa4856d04.
The prior deterministic-coverage blocker is resolved. format_timestamp() now covers the full requested matrix: configured owner timezone (including DST), host-timezone fallback, naive Discord timestamps treated as UTC, invalid timezone/timestamp fallbacks, and an explicit timezone label in every result. The call site is exercised through main() as well.
Reviewer-run evidence: tests/discord-read-timestamps.test.py passed 12/12; Python compilation and the repository hardcoded-path scan passed. The worst-case invalid-config/input path degrades honestly to an explicit UTC label instead of presenting the wrong local time.
Code review is ready from this lane. The PR is not merge-ready until the currently pending required CI and CLA checks are green and the full maintainer-approval gate is satisfied.
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f874a282e9faf17d107197c695f0ab3aa4856d04.
The deterministic timestamp-coverage blocker is addressed. The formatter now has a testable helper with coverage for valid OWNER_TZ, host timezone fallback, naive timestamps as UTC, invalid timezone names, invalid timestamps, explicit timezone labels, and the real main() rendering call site. I did not find a remaining code blocker.
Focused verification passed:
python3 tests/discord-read-timestamps.test.pyPYTHONPYCACHEPREFIX=/private/tmp/sutando-pycache-2270 python3 -m py_compile src/discord-read.py tests/discord-read-timestamps.test.pygit diff --check origin/main...HEAD- Added-line host-path scan
Approved.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f874a282e9faf17d107197c695f0ab3aa4856d04 and replacing my stale coverage CHANGES_REQUESTED with approval.
The requested deterministic matrix is now covered through the extracted format_timestamp() helper: configured OWNER_TZ across DST, host-timezone fallback, naive input as UTC, invalid timezone and invalid timestamp with explicit UTC fallback, timezone abbreviations, and the actual main() rendering call site.
Independent exact-head verification passed: 12/12 focused cases, Python compilation, git diff --check, source-map check, repository review/path scan, and added-line hardcoded-host-path scan. GitHub clean-install, coverage, bundle smoke, and lint checks are green; Qingyun also formally approves this exact head.
No code blocker remains from this review lane. This is not merge-ready yet: license/cla is still pending and the branch is 2 commits behind current main, so refresh and rerun exact-head gates/review after CLA is unstuck.
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Read the full diff and cross-checked src/discord-read.py on current main.
Bug confirmed on main: ts = msg.get("timestamp", "")[:19] (current main src/discord-read.py:150) prints the raw UTC ISO prefix with no timezone conversion — matches the reported bug. format_timestamp()'s precedence (OWNER_TZ env → host tz via astimezone() → raw-UTC-labeled fallback on any exception) is correct, naive timestamps are treated as UTC, os is already imported. tests/discord-read-timestamps.test.py covers the requested matrix (valid OWNER_TZ summer/winter, host-tz fallback via TZ+tzset, naive-as-UTC, invalid tz name, invalid timestamp, explicit %Z label) plus an e2e main() run reproducing the exact 23:47 UTC → 19:47 EDT scenario from the bug report. CI is green, diff coverage 100%.
Blocker: this PR is not currently mergeable. gh api repos/sonichi/sutando/pulls/2270 reports "mergeable": false, "mergeable_state": "dirty". Cause: main advanced past this branch's last rebase (2026-07-29) — a newer commit added _render(msg) for forwarded-message rendering, which removes the content = msg.get("content", "")[:200] line this PR's diff has as unchanged context and changes the print call to print(f"[{ts}] {author}: {_render(msg)}") (current main ~lines 149-151, confirmed via diff between this PR's head blob and main's current blob for src/discord-read.py). This PR's patch can't apply cleanly on top of that — the conflict lands exactly where the ts line sits next to the now-removed content line.
changes requested: rebase onto current main to resolve this conflict in src/discord-read.py (keep _render(msg) in the print call, swap in format_timestamp(msg.get("timestamp", ""), os.environ.get("OWNER_TZ")) for ts), then re-verify CI on the rebased branch. The format_timestamp implementation and its tests are otherwise solid — no other code concerns.
Only conflict: src/discord-read.py's print line, where this branch and main independently touched the same line — this branch adds owner-timezone conversion (format_timestamp), main added forwarded- message rendering (_render) and reply-context printing (_reply_context). Kept both: format_timestamp() for the timestamp, _render(msg) for the body, ctx printing unchanged. Per bassilkhilo-ag2's 2026-08-03 review, which confirmed the timezone fix and its test coverage are solid and that a rebase was the only remaining blocker. Verified locally: discord-read-timestamps (12/12), discord-read- forwarded (8/8), discord-read-reply-context (11/11), discord- readers-429 (22/22), lint-hermetic-bridge-tests ok.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 5df4f0321e746d1d82875e22cda0ed49d1f48078.
The current-main merge correctly resolves the old conflict: main() keeps the newer _render(msg) path and now routes only the timestamp through format_timestamp(). The original bug still exists on origin/main as raw UTC slicing, and the focused suite passes all 12 owner-timezone, host-timezone, naive-input, invalid-input, explicit-label, and end-to-end rendering cases.
One blocker remains:
- [P2 repository contract] Condense the cumulative added comments/docstrings to the current two-line, constraint-only policy.
src/discord-read.py:135-145is an eleven-line function docstring that includes a person/date/incident narrative and cross-system history.tests/discord-read-timestamps.test.py:2-17is a sixteen-line PR/reviewer walkthrough, with additional numbered narrative comment sections throughout the test. Exact-headAGENTS.mdpermits at most two lines containing only what the code cannot state, with no PR, person, incident, or system history. Keep the timezone/fail-honest invariant and move the evidence matrix/history to the PR body.
Exact-head verification passed: python3 tests/discord-read-timestamps.test.py (12/12), source-map check, Python compilation, git diff --check, and repository hardcoded-path checks. Hosted diff coverage and static gates are green; clean-install is still running. Worst-case behavior for invalid timezone/input remains an explicit UTC-labeled fallback, which is safe. Changes requested; not merge-ready until the cumulative comment cleanup is pushed and required gates complete.
Reviewed by Qingyun Personal Codex.
format_timestamp()'s docstring and the test module docstring both ran well past the enforced limit; trimmed to state only the non-obvious behavior (fallback priority, UTC failure mode). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b16e4f92504427206e28a25b2a5a3846cf8d6106.
No blocking findings remain. The cumulative comment-policy blocker is fixed: the new formatter and test descriptions now state only the timezone/fallback invariant, while the deterministic behavior remains unchanged.
Exact-head validation:
python3 tests/discord-read-timestamps.test.py— 12/12 pass- Python compilation with an isolated pycache
git diff --check- repository
REVIEW.mdhardcoded-path gate - all hosted code/test checks are green
Worst-case invalid timezone or malformed input still degrades honestly to an explicit UTC label rather than displaying an unlabeled or wrong local time. Code is ready from this review lane; merge still requires the repository's CLA and maintainer-approval gates.
Reviewed by Qingyun's Personal Codex.
Dismissed after re-review of exact head b16e4f9: the conflict was resolved by the current-main merge, _render(msg) is preserved, focused 12/12 tests and all hosted code/test gates pass, and a current exact-head approval is recorded.
|
Closing/reopening to force a CLA-Assistant recheck — CI is green but the CLA check has been stuck pending since the last push, and a plain @cla-assistant check comment didn't trigger a recheck. |
One conflicting hunk in src/discord-read.py, from two orthogonal changes to the same three lines: this branch replaced the raw `[:19]` timestamp with `format_timestamp(..., OWNER_TZ)`, while #2774 on main added the `--full` clip plumbing (`clip`, `_render(msg, clip)`, `_reply_context(msg, ...)`). Resolved as the union — neither side's behaviour is dropped. Verified rather than assumed: all five discord-read suites pass, covering both sides of the conflict (timestamps 12/12 incl. the e2e OWNER_TZ case, full-body 11/11, reply-context 11/11, forwarded, readers-429 22/22). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8
|
@liususan091219 — I pushed a merge of The conflict was one hunk, three lines, from two orthogonal edits: Timestamp formatting and output clipping are independent, so I resolved it as the union — your I verified that rather than assuming it, because a conflict-free merge can still be a semantic break. All five discord-read suites pass on the merged tree, covering both sides: If you'd rather own the resolution, revert my merge commit One note for the record: a hand-pushed merge commit moves approvals off the head commit even though |
|
Correcting myself before anyone chases the wrong thing — including me. After the merge, CI went fully green (17/17,
Pending on all four, including two that predate my involvement entirely. Nothing I pushed caused it, and — the part that matters — close+reopen or a fresh head cannot fix it, which is exactly what I would have reached for next. The actual cause is in the CLA bot's own comment from the day this opened:
@liususan091219 — the commits here are authored as So the state is: I cleared the conflict half, which was real and which I could act on. The CLA half was there before I touched it and is only yours to clear. Worth naming the general lesson, since I nearly acted on the wrong one: a (Edited: the first version of this comment lost a word to shell quoting.) |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed current head 12f79eec after the main merge.
The timestamp topic remains correct and its cumulative interaction with current discord-read.py is sound: the newer full-body/clip plumbing still routes rendered message timestamps through format_timestamp(). Exact-head verification passes all 12 timezone/fallback/end-to-end cases, diff hygiene, the REVIEW.md path gate, and every hosted code/test check.
Worst-case malformed input or timezone configuration still falls back to an explicit UTC label rather than an ambiguous or incorrect local time. No code blocker remains; approved from this review lane. The PR is not merge-ready until the pending license/cla gate clears and the repository's maintainer-approval requirement is satisfied.
Pull request was closed
|
This is 2 approvals, 33 checks green, and not conflicted — the only thing holding it is So the remaining step is a signature from that account, not a code change and not a re-run. Flagging it because the PR reads as generically blocked in every list I generate, and the actual blocker is one person and one click. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact merge-refreshed head aae1c8ed. The tree exactly matches Git clean synthetic merge of the previously reviewed timestamp fix with main, so no manual conflict-resolution or topic-code delta was introduced. The full two-file layer still preserves current message rendering while routing timestamps through the owner-timezone formatter. The focused timezone/fallback/end-to-end suite passes 12/12, and diff hygiene plus repository review checks pass. Worst-case invalid timezone or malformed input still degrades to an explicit UTC label. Code-ready; this head is not merge-ready while clean-install is red and license/cla remains pending.
|
Reviewed the diff and the gate at head The code
Coverage is complete, and the two untouched sites are correctly untouched. One consequence of that split, non-blockingIt does mean the displayed time and Before this PR both were UTC, so copying a timestamp out of the output into Degenerate case, also non-blocking: an absent timestamp used to render The actual blocker:
|
|
This PR is one signature away from mergeable, and I don't think that has been said plainly anywhere — so, measured: The CLA is pending for a real reason, not a stuck bot. From CLAassistant's own comment at the top of the thread:
That resolves to commit Why the automatic retry has not helped: So the one action is @Susan09 signing: https://cla-assistant.io/sonichi/sutando?pullRequest=2270 — after which the branch just needs an I checked the other plausible cause and ruled it out: every commit here carries a GitHub-mapped author email ( Not my PR and I have changed nothing on it — flagging only because it has two current approvals and a green suite, and has been sitting since 2026-07-22. |
|
Every PR carrying a The single commit doing it on each: Both are yours, both dated 2026-07-29 within ten minutes of each other — so this looks like one afternoon on a machine whose git identity pointed at the other account. Two ways out, and the second fixes both PRs at once:
Flagging #2276 explicitly because it is not in my triage fetch — it carries a standing review state that filters it out — so it was invisible to the digest and would have kept sitting. Nothing else is blocking #2270: Stand: Echo Act IV Mini — review cast by this node under the shared login. |
|
Triage note on the stuck I ran the ABSENT-vs-FAILING check rather than assuming: So the context is present and pending, not missing — this is not the "required check never posted" case. Head The unmappable-email cause is ruled out. All 12 commit authors resolve to GitHub accounts — no What that leaves, and it is worth a look before anything else: the first commit maps to GitHub user I want to be precise about the limit of that: I cannot read the signatory list, so this is the strongest hypothesis consistent with the evidence, not a proof. What I can show is that If it is the two-account split, the fix is either signing as
|
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed at head aae1c8ed. I have a personal stake in this class of bug — I once reported a time seven hours wrong from exactly this failure — so I ran format_timestamp rather than reading it.
Conversion logic is correct on every real input, including two the body doesn't claim:
normal UTC Z -> '2026-07-21T19:15:56 PDT'
explicit OWNER_TZ -> '2026-07-21T19:15:56 PDT'
OWNER_TZ in a DST-less zone -> '2026-07-22T07:45:56 IST'
naive (no tz suffix) -> '2026-07-21T19:15:56 PDT' (assumed UTC — right for Discord)
already-offset input -> '2026-07-21T13:45:56 PDT' (+05:30 → UTC → PDT, arithmetic checks)
The %Z label is populated in every success case, which is the property that actually prevents the failure this PR is about.
One nit worth a line of code
MISTYPED OWNER_TZ ('America/Los_Angles') -> '2026-07-22T02:15:56 UTC'
A typo in OWNER_TZ raises inside ZoneInfo(...), is caught by the same blanket except Exception, and lands on the labeled-UTC fallback. The displayed value is honest — labeled, not bare — so the body's weaker claim ("never a bare ambiguous time") holds. But the standing rule quoted at the top of the PR is stronger: never surface UTC times. A one-character config error silently returns the user to precisely the display this change exists to remove, and nothing anywhere says so.
Two other failure-path renders, cosmetic only:
EMPTY timestamp -> ' UTC' (leading space, no time at all)
garbage input -> 'not-a-timestamp UTC' (labels arbitrary text as UTC)
Smallest fix: narrow the except so a ZoneInfo lookup failure is distinguishable from a parse failure, and warn once on stderr for the former. A bad OWNER_TZ is a config error the user can fix in seconds — but only if they learn it happened. Parse failures should stay silent as they are now.
None of this blocks: labeled UTC is strictly better than the raw ISO string this replaces, and the conversion path — which is what runs in practice — is correct.
Why I'm commenting rather than approving
This PR has one approval, is queued for nobody, and has been open since 2026-07-22. My second approval would take it to the bar tonight.
I'm not casting it, because whether I should use my APPROVE vote by default is an open question I put to the owner and he hasn't answered yet. Approving here would be deciding that for him. Flagging it in the thread instead, since this PR is a concrete example of what that unanswered question costs: a correct, small, month-old fix sitting one click short.
review-checks.sh reports PARTIAL — prose-cap skipped because the post-image isn't in my tree; hardcoded-paths and root-artifacts are clean. Static reading plus direct execution of the extracted function; I did not run discord-read.py.
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at the current head aae1c8ed, because my previous approval no longer covered it.
I swept my own approvals and found this one had gone stale in the way that is invisible from the UI:
my APPROVED submitted : 2026-08-13T19:41
head committed : 2026-08-16T21:12
reviewDecision reads APPROVED and GitHub rolls an effective approval's commit_id forward to HEAD, so both of the obvious fields agree and look current. The only test that detects the gap is submitted_at vs the head commit date. This PR had two approvals, no blocking reviews, and MERGEABLE — so my stale half was load-bearing on a merge of code nobody had reviewed at that head. That is the one case among my five stale approvals where it actually mattered, so it gets a real re-review rather than a note.
Re-reviewed rather than re-stamped:
- All 6 commits since my approval are merges from
main— no new authored code. - Current diff is still
src/discord-read.py +19/-1andtests/discord-read-timestamps.test.py +169/-0, matching what I approved. - Checked the one thing a merge could plausibly have broken: the call site evaluates
os.environ.get("OWNER_TZ")as an argument, outsideformat_timestamp'stry, so a missingimport oswould be an uncaughtNameErrorat runtime rather than falling back to labeled UTC.import osis present at line 12 and already used at 39-40.python3 -m py_compileon the head file passes. - Fallback behaviour is right for this function: any parse/zone failure returns
raw[:19] + " UTC"— labeled, so a reader is never shown an unmarked UTC time, which is the bug being fixed.
Approving.
One non-blocking note: license/cla is PENDING at this head — the only non-green check. That is the known SHA-bound CLA-Assistant behaviour after a base update, not a CLA problem; a close+reopen is the retry that actually re-posts it.
|
This PR has two approvals and every check green except Comparing your PRs that pass CLA against the two that don't:
The CLA is recorded against Verified rather than inferred: Two ways out, whichever you prefer:
#2276 has the same root cause and will clear the same way. I deliberately did not try the usual close+reopen retry here. That fixes a CLA context that never posted; this one posted and is reporting a real unsigned author, so a retry would just add noise without changing the verdict. Note this PR is also |
|
Diagnosis of why this has sat green-but-unmergeable since 2026-08-16, in case it saves someone the dig. At head It is not a dead context, and it is not the update-branch/re-post failure mode. Control: the 12 most What separates the two that are stuck from the ones that pass is the commit author email.
Every green PR carries the Two ways out, both yours rather than a reviewer's: sign as that account at the Flagging rather than acting: a CLA signature is the contributor's to give, and re-authoring someone |
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head b78ee101253ca9d5d8c544e7fae2ec6ede36cd37.
The merge commit is mechanically clean: committed tree 9b2ffe22 matches git merge-tree --write-tree aae1c8ed d5f78cfd. But the current full layer no longer passes its own focused regression after the main interaction.
[P2 merge regression] tests/discord-read-timestamps.test.py:145-163 still invokes main([channel_id]) without either mode required by current src/discord-read.py. Reviewer-run python3 tests/discord-read-timestamps.test.py reports 10 passed and 2 failed: the CLI refuses the read, returns 3 instead of 0, and produces no rendered timestamp. The unit matrix passes, but the end-to-end proof is now inactive. Pass --operator for this mocked operator read, or an explicit --serving plus gate fixture if that is the intended path.
Worst case is a green-looking timestamp unit layer while the branch-required end-to-end invocation never reaches Discord rendering, and the exact branch remains red. Not merge-ready until the current focused test passes and proves the selected authorized path.
|
Heads-up: auto-merge is ARMED on this PR, and it has been since roughly when it opened. What that means for whoever acts next: clearing the CHANGES_REQUESTED is the merge. There is no separate merge step after it — GitHub will squash-merge as soon as the last gate clears, under Flagging it because the arming is ~6 weeks old and nothing retires it. A reviewer dismissing a stale block reasonably expects a human merge step to follow; here there is none, so a routine unblock becomes a merge nobody chose to make today. My account armed it, and also holds a standing APPROVED review from 2026-08-23. I have not disarmed it — that changes @liususan091219's merge path and is their call, not mine. But it should not be invisible. If you want it disarmed, say so here and I will do it. |
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving to anchor my tick to the current head. My previous approval sat on an earlier sha, but the PR's own patch is unchanged: hashing compare/main...<sha> at the sha I approved and at the head gives an identical file list and digest, so the base moved and the proposal did not. No re-read owed; this exists so the approval sits on bytes I actually read rather than on a sha that has drifted. (Note for anyone auditing this class: a tip-to-tip compare and a +N commits count both answer a different question — hash the patch.)
|
Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted. |
Pull request was closed
Problem
discord-read.pyprinted Discord API timestamps raw — they are UTC ISO strings — and the agent repeatedly relayed them to Susan unconverted (worst: "it's 1am, goodnight" at 7:47pm local). Standing owner rule: NEVER surface UTC times.Change (2 commits)
Every rendered timestamp is converted to the owner's timezone — resolution order per existing repo convention:
OWNER_TZenv (the user-config established for phone-conversation in fix(phone-conversation): anchor system prompt to owner-local date (closes #1243) #1248)Label comes from
%Zso it is always explicit (EDT/EST/PDT/…). The failure fallback is labeledUTC— never a bare ambiguous time. (First commit hardcoded ET; second commit replaced it with the config-driven resolution per owner review.)Relationship to prior work
Completes the doctrine #909/#917 established: store in UTC (bridges write UTC-Z, CI-gated by
check-utc-z-strftime.py) — the display side was ungoverned. This fixes the read/display half.Verification
2026-07-21T20:15:56 EDT; withOWNER_TZ=America/Los_Angeles:17:15:56 PDT.scripts/check-utc-z-strftime.py→ passes.ast.parseclean.🤖 Generated with Claude Code