Repository navigation
fix(friction-detector): replace deprecated datetime.utcnow() with timezone-aware now(UTC) - #2003
Conversation
…ezone-aware now(UTC) datetime.utcnow() was deprecated in Python 3.12 and scheduled for removal. Both `now` and the parsed `updatedAt` are now timezone-aware (UTC), so their difference is still correct. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 41%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…son for diff-coverage gate Adds two tests that mock subprocess.run to exercise the timezone-aware datetime comparison path without a live gh token, satisfying the >= 95% diff-coverage gate on PR sonichi#2003. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
…son for diff-coverage gate Adds two tests that mock subprocess.run to exercise the timezone-aware datetime comparison path without a live gh token, satisfying the >= 95% diff-coverage gate on PR sonichi#2003. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
|
@cla-assistant check |
2 similar comments
|
@cla-assistant check |
|
@cla-assistant check |
|
CLA is pending because commits include one from @ruiwang (sha |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed — fix is correct, one process blocker. The timezone-aware migration is exactly right (aware now + aware parse, dropping the tzinfo=None strip — no more naive/aware comparison risk), and the mocked-gh tests exercise the new path. All functional checks are green; the only red is license/cla stuck at "not signed yet" — this looks like the known CLA-assistant stall. Suggest a close+reopen (comment-triggering @cla-assistant check has been dead in past cases) to re-fire it; not mergeable until that's green.
… for Ubuntu CI (sonichi#2029) * test(shell-ci): fix two ubuntu-portability failures from shell-ci-known-failures Two tests fail on ubuntu-latest CI (wired in by sonichi#1948) due to macOS-specific tooling; both pass on macOS today but gate CI on Ubuntu. start-cli-claude-config-dir.test.sh (2 failures): 1. Ambient CLAUDE_CONFIG_DIR leak — the test runner session has CLAUDE_CONFIG_DIR set, which leaked into the subprocess and broke "not set" assertion. Fix: add `unset CLAUDE_CONFIG_DIR` in setup_sandbox() alongside the HOME/SUTANDO_WORKSPACE exports that already clear the environment. 2. /var/folders vs /private/var symlink mismatch — macOS mktemp returns /var/folders/… but cd+pwd -P resolves to /private/var/folders/…; sutando-config.sh uses the physical path, so the expected vs actual CLAUDE_CONFIG_DIR differed. Fix: resolve SANDBOX to its real path (cd+pwd -P) immediately after mktemp. 3. Stale expected path — expected was keyed on $SUTANDO_WORKSPACE (deprecated v0.8 env-var, no longer read by the resolver) instead of the config-derived $REPO_FAKE/workspace. Fix: align expected to what sutando-config.sh actually computes. startup-env-token-persist.test.sh (would fail on Ubuntu): - stat -f '%Lp' (BSD mode format) and stat -f '%m' (BSD mtime) are macOS-only. Fix: replace both with equivalent Python one-liners (stat.S_IMODE + os.stat.st_mtime) that work on macOS and Linux without any extra deps. After this PR, both suites should be removable from tests/shell-ci-known-failures.txt once that file lands (via sonichi#1948). Closes two items from sonichi#1961. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw * test(shell-ci): fix sutando-migrate.test.sh — wrong DEST + BSD stat + pipefail exit Three bugs caused all 7 assertions to fail: 1. Wrong DEST — test used `SUTANDO_WORKSPACE="$DEST" --respect-env` but the workspace resolver ignores $SUTANDO_WORKSPACE since sonichi#1440. The backup landed in the real workspace instead of the temp fixture. Fix: use SUTANDO_MIGRATE_DEST (the documented E2E test hook symmetric with SUTANDO_MIGRATE_SRC_{A,B,C}). 2. Early exit via set -e — `grep -E "^sutando-migrate: backup"` exits 1 when the dest is empty (the backup line reads "dest empty; placeholder backup → …"). With `set -uo pipefail`, the INITIAL_BACKUP_ID assignment aborted the script before the assertion block ran. Fix: broaden the pattern to "migration-backup-.*\.tar\.gz" and add `|| true`. 3. BSD-only stat — `stat -f %m` is macOS-only; GNU stat uses `-c '%Y'`. Affects mtime comparison for notes/shared.md on Ubuntu. Fix: Python `os.stat().st_mtime` (portable, already used in startup-env-token-persist and start-cli fixes on this branch). All 14 assertions now pass locally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
…onichi#2010) * feat(skills): report-feedback skill (single reporting path) The user can report a bug / feature request / feedback by asking Sutando from ANY surface — chat, Discord, Telegram, or voice (via task delegation). One skill, not a per-surface tool, so there's no duplication. - skills/report-feedback/report-feedback.py posts to the cloud /api/feedback route (mirrors into GitHub issues) with kind/severity/title/body + a diagnostic context (platform + a tail of recent <workspace>/logs). - Reads cloud-auth.json on disk (falls back to the metering env the desktop injects); prints NOT_SIGNED_IN + exits 2 when signed out, ERROR on API failure — never leaves the agent guessing. - Owner-tier only. Replaces the earlier voice-only report_issue inline tool (dropped) per owner: the skill covers voice too via delegation, avoiding a duplicate capability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zp1T5KEdGsfNRFSLhY3fK * fix(report-feedback): read the cloud token the desktop actually writes The auth probe used the wrong path (state/auth/) and required a `signedIn` field. The desktop's readCloudAuth (electron/ipc.cjs) stores the record at <workspace>/cloud-auth.json and treats "signed in" as simply having a `token` (no signedIn field). Match that: read <workspace>/cloud-auth.json, also probe the packaged-app canonical workspace (~/.sutando/repo/workspace) so the skill finds the token across the dev/packaged split, and gate on token presence. Verified end-to-end: a signed-in submission returns 200 and lands in the feedback funnel + mirrored GitHub issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zp1T5KEdGsfNRFSLhY3fK * fix(report-feedback): announce+redact logs, harden tier note, fix null body Addresses review on sonichi#2010 (qingyun-wu): - SKILL.md: instruct the agent to ANNOUNCE the log attachment before sending ("I'll attach recent diagnostic logs unless you'd rather I didn't") and honor --no-logs — an informed opt-out for voice/chat, matching sonichi#57's checkbox. - report-feedback.py: add _redact() backstop over the log excerpt (Bearer tokens, token=/api_key=/secret= values, common key formats sk-/xoxb-/ghp_/ AKIA, and the home-dir username). Private-Slack mirror makes this non-blocking, but /api/feedback can also open GitHub issues, so scrub before it leaves. - SKILL.md access-tier: state explicitly that non-owner Discord/Slack/Telegram route to sandboxed codex (no cloud token) and cannot reach this skill, so a non-owner can't ship the owner's logs into an issue. Also fixes a real bug found while testing: an empty --body sent body:null, which /api/feedback rejects (400 invalid_payload, 'Expected string, received null'). Now sends a string (falls back to the title), matching the desktop form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DmsnVs5ffwHXdrQsLoZUCb * fix(report-feedback): probe state/auth/cloud-auth.json (M1 canonical) first Post-M1 the cloud auth record lives at <workspace>/state/auth/cloud-auth.json. Add it as the first probe; retain the pre-M1 root path as a 30-day reader fallback per the workspace contract. Mirror the same M1+fallback pair for the packaged-app workspace so the skill finds the token regardless of checkout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw * fix(report-feedback): match real AWS access keys in _redact() (no separator) AKIA keys have no separator between the prefix and body (AKIAIOSFODNN7EXAMPLE), so the shared [_-] pattern never matched them. Split out a dedicated AKIA[A-Z0-9]{16} pattern that fires correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw * fix(report-feedback): redact Slack app tokens * test(report-feedback): cover main flow, cloud-auth branches, and logs excerpt Raises diff coverage on report-feedback.py from 35% to 100%, clearing the Coverage Gate on sonichi#2010. Adds tests for resolve_workspace (import + fallback), the read_cloud_auth metering-env / dedup / malformed-file branches, logs_excerpt (present / absent / unreadable), and main() (blank-title exit, not-signed-in exit, successful POST with and without logs, HTTPError and generic-error exits) via mocked urlopen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DC4oxV4ZwnKTKvE6Hb28Yo * fix(report-feedback): redact Google API keys --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
…d fabrication (sonichi#2014) * fix(voice): add explicit local-machine identity to prevent cloud fabrication Addresses sonichi#2011 — when asked where Sutando runs, Gemini was defaulting to its training-data answer ("cloud-based AI"). Add an authoritative system-instruction line so the model answers correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUA5TZgDa7VmFcNtPka38t * test: update voice-behavior-anchors fixture for identity instruction Regenerate the instructions hash after adding the local-machine identity line to buildInstructions() (deliberate prompt change for sonichi#2011). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUA5TZgDa7VmFcNtPka38t * fix(phone): add explicit local-machine identity to prevent cloud fabrication Same issue as voice (sonichi#2011) — phone agent also lacked an authoritative statement about running locally. Adds the same system-instruction line to the inbound/outbound phone session path in buildPhoneInstructions(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUA5TZgDa7VmFcNtPka38t * test: update phone-behavior-anchors fixture for identity instruction Regenerate hashes after adding the local-machine identity line to buildPhoneInstructions() (deliberate prompt change for sonichi#2011). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUA5TZgDa7VmFcNtPka38t --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
4457437 to
dd54841
Compare
|
Automated mergeability review (2026-07-09) Signal: NOT MERGEABLE Current blocker(s):
This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Delta review of dd548415 (since my 2026-07-09 19:00 review). CI: all checks green — including license/cla, which was the single blocker in my previous review. That is now resolved.
On the commits added since my review: a602a1bd (#2029 test fixes), e5c130ae (#2010 report-feedback), and 9d12a727 (#2014 voice identity) are copies of already-merged main PRs brought in via the "Merge branch 'main'" commit — they are not new branch work. Because those copies carry different SHAs than the upstream merge commits, they inflate the three-dot diff with files that look like PR changes (report-feedback skill, phone/voice config, behavior anchors, test scripts). I compared blob SHAs of every such file between this PR's head and upstream main: all identical — no conflict-resolution drift, so merging this PR nets only the friction-detector change.
The actual PR content (src/friction-detector.py:123-125 + the mocked-gh test) is unchanged from what I already reviewed: aware now(timezone.utc) compared against an aware parsed updatedAt with the tzinfo=None strip dropped — correct.
LGTM (comment only). Non-blocking suggestion: next time prefer merging upstream main directly (or update-branch via GitHub) rather than a fork-local main with re-created commits — it keeps the PR diff limited to the branch's own work and makes review much cheaper. Branch currently reports merge state BEHIND; an update-branch before merge would also collapse the diff back to the two friction-detector files.
liususan091219
left a comment
There was a problem hiding this comment.
Triage change-request (owner-approved batch, 2026-07-12 sweep). Conditions found on the current diff:
- body claims what the diff does not deliver — Body says pure warning fix, but diff adds phone identity prompt and new feedback skill.
- live-test evidence required (voice/vision policy) — Phone voice prompt behavior changed; provide call transcript or live-test evidence.
Justification: kb triage standards (source-grep test standard, voice/vision live-test policy, necessity/coverage checks) — see sonichi/sutando-pr-triage kb/.
Address the asks and push; re-request review to clear this. If a finding is wrong, reply with the counter-evidence and it will be withdrawn.
cr-reason: false_claim+live_test_required
liususan091219
left a comment
There was a problem hiding this comment.
Triage change-request (owner-approved batch, 2026-07-12 sweep). Conditions found on the current diff:
- body claims what the diff does not deliver — Body says pure warning fix, but diff adds phone identity prompt and new feedback skill.
- live-test evidence required (voice/vision policy) — Phone voice prompt behavior changed; provide call transcript or live-test evidence.
Justification: kb triage standards (source-grep test standard, voice/vision live-test policy, necessity/coverage checks) — see sonichi/sutando-pr-triage kb/.
Address the asks and push; re-request review to clear this. If a finding is wrong, reply with the counter-evidence and it will be withdrawn.
cr-reason: false_claim+live_test_required
Superseded by the corrected change-request above (live-test ask withdrawn per owner policy 2026-07-13; the false_claim ask stands).
liususan091219
left a comment
There was a problem hiding this comment.
Triage change-request (owner-approved batch, 2026-07-12 sweep). Conditions found on the current diff:
- body claims what the diff does not deliver — Body says pure warning fix, but diff adds phone identity prompt and new feedback skill.
Justification: kb triage standards (source-grep test standard, voice/vision live-test policy, necessity/coverage checks) — see sonichi/sutando-pr-triage kb/.
Address the asks and push; re-request review to clear this. If a finding is wrong, reply with the counter-evidence and it will be withdrawn.
cr-reason: false_claim
Superseded by the corrected change-request above (live-test ask withdrawn per owner policy 2026-07-13; the false_claim ask stands).
|
Addressed Same stale-branch root cause as #1966: the bundled report-feedback skill, phone/voice config, and fixture edits had already merged to main via their own PRs — the three-dot diff was showing stale duplicates. Merging Files-changed is now exactly the claim: |
|
Context: this is the last Suggest pairing it with a one-line CI guard that bans |
|
LGTM. I diffed this against Qingyun's Personal Codex Agent |
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: ADDRESSED — Current diff only changes timezone-aware datetime handling in
friction-detector.pyand adds relate
All asks addressed — the stale CHANGES_REQUESTED will be dismissed in the next owner-approved batch.
Dismissing: re-review confirms all CR asks addressed (datetime.utcnow → timezone-aware now(UTC); it's the last utcnow straggler after #1385, now zero repo-wide). Owner-directed dismissal 2026-07-13.
liususan091219
left a comment
There was a problem hiding this comment.
Approving (owner-directed). The false_claim CR is verified resolved: the PR is now scoped to exactly what its body describes — src/friction-detector.py (datetime.utcnow → timezone-aware now(UTC)) + its test, with no extra phone-identity/feedback-skill changes. Before/after confirmed: -W error raises no DeprecationWarning, 11 tests pass. This also clears the last utcnow() in the repo.
review-reason: false_claim resolved (body/diff aligned), verified before/after, owner-directed approve 2026-07-13
|
@cla-assistant check |
Summary
datetime.utcnow()was deprecated in Python 3.12 and scheduled for removal in a future version; it emits aDeprecationWarningat runtime on 3.12+.nowand the parsedupdatedAtare now timezone-aware (UTC), so(now - updated).daysstill produces the correct stale-day count.Test plan
python3 tests/friction-detector-pending-questions.test.py— all 11 tests passpython3 -W error src/friction-detector.py— no DeprecationWarning raised🤖 Generated with Claude Code
https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh