fix(feedback): guard against non-array data + widen route error handling

[gfrankgva]

Summary

Fixes #93 — POST /feedback returns 500 Internal Server Error when feedback.json contains valid but non-array JSON data.

Two-layer fix:

• src/core/FeedbackManager.ts — loadFeedback() now validates the JSON.parse() result with Array.isArray() before returning. Non-array data (objects, null, numbers, strings) returns [] instead of being passed through, preventing TypeError when downstream code calls .slice(), .some(), .filter().

• src/server/routes.ts — The POST /feedback route handler's try/catch now wraps the entire handler body (quality validation + anomaly detection + submit), not just the submit() call. Any TypeError from loadFeedback() via validateFeedbackQuality() is now caught and returns a proper 500 JSON response.

Test plan

• 8 new unit tests in tests/unit/feedback-loadFeedback-guard.test.ts
  • loadFeedback() returns [] for object JSON ({})
  • loadFeedback() returns [] for null
  • loadFeedback() returns [] for number (42)
  • loadFeedback() returns [] for string ("hello")
  • loadFeedback() returns the array for valid array JSON
  • loadFeedback() returns [] for missing file
  • loadFeedback() returns [] for invalid JSON
  • validateFeedbackQuality() does not throw with corrupted data
• Existing feedback-security.test.ts tests pass (no regressions)
• TypeScript compiles cleanly (tsc --noEmit)

🤖 Generated with Claude Code

[vercel]

@TESTPERSONAL is attempting to deploy a commit to the sagemind Team on Vercel.

A member of the Team first needs to authorize it.

[JKHeadley]

[Review withdrawn — identity error]

This review was accidentally posted under @JKHeadley's account due to a tooling identity-guard bypass on the reviewer agent. The review content was generated by Echo (the AI reviewer agent) and should have been posted under @EchoOfDawn. Reposting under the correct identity below. Apologies for the noise.

[EchoOfDawn]

Echo's Review — PR #124

Note: there is an earlier redacted review on this PR posted accidentally under @JKHeadley's account due to a tooling identity-guard bypass. This is the actual review.

Recommendation: merge
Value: high | Strategy: comment-only

Summary

Two-layer fix for issue #93 (POST /feedback → 500). Adds an Array.isArray() guard to FeedbackManager.loadFeedback() and widens the route handler's try/catch to also cover the quality + anomaly-detection phases that run before submit(). Tight, well-scoped, and the diff stays inside the affected handler.

Strengths

• Correct root-cause analysis. validateFeedbackQuality() calls loadFeedback().slice(-50) for duplicate detection, so a non-array feedback.json ({}, null, 42, "x") does throw a TypeError before the original narrow try/catch is reached. Both fixes are necessary in combination.
• Defensive symmetry: loadFeedback() already returned [] for a missing file and for parse errors, so returning [] for non-array data matches existing behavior. No surprise in the contract.
• 8 focused unit tests covering object / null / number / string / valid array / missing file / invalid JSON, plus a route-level smoke that validateFeedbackQuality no longer throws on corrupted state. The last test is the one that actually proves the bug is gone.
• Includes the side-effects review note under upgrades/side-effects/ and a converged spec under docs/specs/ — matches instar's existing layout.

Concerns

• (minor) The spec frontmatter has approved: true, approved-by: gfrankgva, approval-note: .... Spec approval in this repo is a maintainer signal, not a self-attestation. Not a code issue and easily corrected on merge — flagging so future PRs from any contributor leave approved empty.
• (minor) No regression test at the HTTP layer asserting the 500 → 201 path with a corrupted feedback.json. The unit-level proof via validateFeedbackQuality is sufficient evidence of the fix, but a route-level test would lock in the original repro from #93.

Test Coverage

8 new unit tests, all targeted at the failure modes from #93. Existing feedback-security.test.ts is unmodified and unaffected. Full CI is green (Build, Type Check, all unit shards on Node 20 + 22, Integration, E2E). The only failing check is Vercel's "Authorization required to deploy" — that's an org-side OAuth gate, not a code signal.

Security Findings

None. The fix is purely defensive and narrows no existing validation. The widened try/catch doesn't swallow the existing 422 (quality) or 429 (anomaly) early-returns; those return before the catch can ever observe them.

Verdict

Recommend merge. Solid bug fix from a contributor with 4 prior merges (#40, #41, #43, #108), small surface area, real reproducer in #93, no architectural risk. Final approve + merge stays with @JKHeadley.

Next Steps

• @JKHeadley to review and merge if it looks right to you.
• Optional follow-up (no need to block this PR): clear approved in the spec frontmatter on merge, or do it in a tiny housekeeping commit afterwards.

---

Automated review by Echo — instar's developer agent. This review was generated by an AI system. For questions or concerns, please tag @JKHeadley.

[gfrankgva]

Addressed both review items from Echo:

1. Spec frontmatter — removed self-attested approved/approved-by/approval-note fields. Approval stays with @JKHeadley.
2. HTTP-layer regression test — added route-level test proving POST /feedback no longer returns 500 when feedback.json contains non-array data ({}).

CI should confirm shortly.

[JKHeadley]

[Review withdrawn — identity error]

This review was accidentally posted under @JKHeadley's account due to the reviewer-agent failing to switch the active gh auth account to @EchoOfDawn before posting. The review content was generated by Echo (the AI reviewer agent). Reposting under the correct identity below. Apologies for the duplicate noise.

[EchoOfDawn]

Echo's Review — PR #124 (Updated — supersedes previous)

Note: an earlier copy of this review was accidentally posted under @JKHeadley's account due to a tooling identity error. That review has been redacted. This is the actual review.

Recommendation: merge
Value: moderate | Strategy: comment-only

Summary

Re-review on ced19df after @gfrankgva pushed two follow-up commits addressing the minor concerns from my previous review. Both are properly resolved.

What Changed Since Last Review

• Spec frontmatter — approved: true (with self-attested approved-by: gfrankgva + approval-note) is now approved: false, with the self-attestation fields removed. Approval signal is back where it belongs (with @JKHeadley).
• HTTP-layer regression test — tests/integration/feedback-corrupted-json.test.ts (new, 119 lines) drives the full request → route → FeedbackManager → response pipeline against a feedback.json containing {}. Two assertions:
  1. The status is not 500 (the original repro from #93).
  2. A valid submission returns 201 with an id, proving the widened try/catch doesn't break the happy path on corrupted state.

That's the test I asked for — it locks in the original bug repro, not just the unit-level fix.

Concerns

None remaining. Both follow-ups are clean.

Test Coverage

8 unit tests + 2 route-level integration tests. CI is green across the board on ced19df (Build, Type Check, Integration, E2E, all unit shards Node 20 + 22). The Vercel check is the unrelated org-side OAuth gate, not a code signal.

Verdict

Recommend merge. Both prior concerns addressed promptly and correctly. Final approve + merge stays with @JKHeadley.

Next Steps

• @JKHeadley to merge when convenient.
• Thanks to @gfrankgva for the quick turnaround on the follow-ups.

---

Automated review by Echo — instar's developer agent. This review was generated by an AI system. For questions or concerns, please tag @JKHeadley.