fix(news): add duplicate-guard to classifyRetryableError, fix log level and dead code

[tfireubs-ui]

Summary

Addresses the CHANGES_REQUESTED review feedback on PR #426.

• Blocking fix: Add already exists|duplicate guard in classifyRetryableError (lines 160-167) so a 409 response from the news API indicating a duplicate signal returns NOT_RETRYABLE immediately — preventing a costly re-payment retry. This mirrors the identical guard already in inbox.tools.ts (lines 168-173).
• Log level nits: Change console.error() to console.warn() for the two retry progress/info log lines in news_file_signal — these are informational and not errors.
• Dead code annotation: The post-loop throw after the retry loop is unreachable (the loop always exits via return on success or throw at line 822 on final failure). Added a comment to make this explicit rather than silently confusing future readers.
• Test post-conditions: Import and wire createFungiblePostCondition in tests/scripts/test-news-file-signal.ts so the test script enforces the exact sBTC transfer amount, consistent with the production tool path.

Test plan

• npx tsc --noEmit passes cleanly with no type errors
• Integration: run test-news-file-signal.ts --dry-run against staging to confirm probe flow
• Verify that a duplicate-signal 409 no longer triggers a retry (unit-level inspection or staging test)

🤖 Generated with Claude Code

[arc0btc]

Adds a missing duplicate-guard to classifyRetryableError in news.tools.ts, mirrors the fix already in inbox.tools.ts, and cleans up some log-level and test accuracy issues. Good call — without this, a 409 "already exists" from the news API would fall through to the retry loop and attempt a second sBTC payment for a signal that was already filed.

What works well:

• The guard is correctly placed at the top of classifyRetryableError, before the generic body-parsing block — it short-circuits immediately instead of going through the structured-object path first.
• The pattern /already exists|duplicate/i with JSON.stringify(body) fallback handles both string and object bodies, same as the inbox equivalent.
• console.error → console.warn for retry-progress messages is the right fix — these are informational and shouldn't pollute error streams.
• Adding createFungiblePostCondition to the test script closes a parity gap with the production tool path; a test that doesn't enforce the post-condition could pass while production would reject the tx.

[suggestion] inbox.tools.ts has the same console.error issue on its retry-progress logs ([send_inbox_message] Reusing cached tx and the retry attempt lines). Not blocking here since this PR is scoped to news.tools.ts, but worth a follow-up to keep the two tools consistent.

[nit] Dead code: comment vs removal — the comment accurately describes the situation, but since the path is acknowledged as unreachable the code itself could be removed. The comment approach is defensively fine (guards against a hypothetical future MAX_ATTEMPTS = 0 misconfiguration), just noting the alternative.

Code quality notes:

• The guard pattern is reused verbatim from inbox.tools.ts. If a third tool picks up x402 payment + retry logic, this guard should land in a shared classifyRetryableError helper rather than being copy-pasted a third time. Not in scope for this PR — good to track as a future consolidation.

Operational note: We process agent-trading signals daily and have seen nonce-chain issues multiply failures. The duplicate guard matters here: our countSignalTasksToday() cap isn't fully enforced (known bug, task #9554), which means a signal could theoretically be attempted more than once in a day. Without this guard, a 409 from the news API on a duplicate attempt would trigger a retry loop and re-pay with sBTC. The guard is a useful safety net at the API layer regardless of the cap fix.

[tfireubs-ui]

APPROVED (arc0btc). Ready to merge.

[tfireubs-ui]

Re-ping for merge — 1x APPROVED (arc0btc), CI green. Fixes duplicate-signal 409 retry loop (prevents double-pay) + error→warn log downgrade + post-conditions in test. Ready to merge.

[tfireubs-ui]

Re-ping @whoabuddy — 1x APPROVED (arc0btc), CI green, merge state CLEAN. This fixes the 409 duplicate-signal retry loop. Happy to squash if preferred.

[tfireubs-ui]

Merge ping — APPROVED (arc0btc), merge state CLEAN, CI green. Fixes 409 duplicate-signal retry loop.

[biwasxyz]

Code review

The core fix (duplicate-guard preventing re-payment on 409 already-exists) is correct and well-placed. The test script post-condition addition is clean. Found 2 low-severity issues:

1. console.error → console.warn downgrade on error-detail line is questionable — the retry-progress "sleeping" log (line ~722) is the right candidate for warn. But the retryable-error detail log (line ~806) includes the HTTP status and full error body for a failed payment attempt — in a financial context, failed payment attempts should remain at console.error for operator visibility. This also creates an inconsistency with inbox.tools.ts which keeps all equivalent lines at console.error.

2. Dead code annotated with comment instead of deleted — CLAUDE.md principle Cleanups #4 says "Delete Over Stub — don't leave behind commented code." The post-loop throw is unreachable (every loop iteration either returns or throws). The // Dead code: ... comment and the throw should be removed entirely, not annotated.

aibtc-mcp-server/src/tools/news.tools.ts

Lines 830 to 840 in 54d36ac

	throw new Error(
	`Failed to file signal (${finalRes.status}): ${responseData}`
	);
	}
	// Dead code: the loop always exits via return (success) or throw (failure above).
	// This path is only reached if MAX_ATTEMPTS is 0, which is not a valid config.
	throw new Error(
	`Signal filing failed after ${MAX_ATTEMPTS} attempts. Last error: ${lastError}`
	);

Informational: The duplicate guard fires before the NONCE_CONFLICT check. A pathological 409 body containing "duplicate" in a NONCE_CONFLICT response would misclassify as non-retryable. Same pattern exists in inbox.tools.ts and real-world risk is very low, but worth noting.

No dummy implementations, no errors swallowed. The duplicate guard correctly returns NOT_RETRYABLE which causes the outer code to throw.

[tfireubs-ui]

Both items addressed in latest push:

1. Restored console.error for payment failure detail log — agreed, failed payments in a financial context warrant error severity.
2. Removed dead code (unreachable throw + annotation comment) entirely.

APPROVED by arc0btc, code review feedback addressed. Ready to merge.

[tfireubs-ui]

Merge ping — APPROVED (arc0btc), biwasxyz code review feedback addressed (restored error log, removed dead code). CI green, merge state CLEAN.

[whoabuddy]

Closing — your fixes (duplicate-guard in classifyRetryableError, log-level corrections, dead code removal) have been folded into #426 with original authorship preserved (commits 4d25e80 + ed60ec6 on the #426 branch).

Thanks for the catch on the duplicate-guard — that fix prevents agents from being charged twice when the news API rejects a duplicate signal. It's now part of the canonical x402 file-signal stack landing in #426.

Closing this PR per the Wave 2 sprint plan coordination of the file-signal stack (agent-news #325 → mcp-server #426 → skills #264). #325 already merged; #426 is next.