Skip to content

Implemented #6 plan: 3 fixes, 30 tests#10

Open
fnandop wants to merge 15 commits into
mainfrom
opencode/issue6-20260503200820
Open

Implemented #6 plan: 3 fixes, 30 tests#10
fnandop wants to merge 15 commits into
mainfrom
opencode/issue6-20260503200820

Conversation

@fnandop

@fnandop fnandop commented May 3, 2026

Copy link
Copy Markdown
Owner

Implemented the last plan from issue #6. Changes made:

  1. src/services/auth.js — Replaced DEMO_PASSWORD_HASH with pre-computed static bcrypt hash of password123
  2. src/index.js — Added clearTimeout(forceTimeout) on successful shutdown and forceTimeout.unref()
  3. src/__tests__/auth.test.js — Added test verifying token expires after JWT_EXPIRES_IN elapses (using jest.useFakeTimers())
  4. Test count — Verified correct counts: 21 auth tests, 9 weather tests, 30 total

The opencode infrastructure will automatically push and create/update the PR.

Closes #6

github run

fnandop and others added 15 commits May 3, 2026 11:37
Co-authored-by: Copilot <copilot@github.com>
This reverts commit 21146ae.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: fnandop <fnandop@users.noreply.github.com>
@fnandop fnandop mentioned this pull request May 3, 2026
@fnandop

fnandop commented May 3, 2026

Copy link
Copy Markdown
Owner Author

Author identity unknown

*** Please tell me who you are.

Run

git config --global user.email "you@example.com"
git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident name (for runner@runnervmeorf1.0y4ggixqpiae5jt5tzqp2bomxe.ex.internal.cloudapp.net) not allowed

github run

@fnandop

fnandop commented May 3, 2026

Copy link
Copy Markdown
Owner Author

/oc review this pr

@fnandop

fnandop commented May 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review for #10

Verdict: Request Changes

Blocking Issue

Flawed token expiration test (src/__tests__/auth.test.js:98-125)

  • JWT_EXPIRES_IN is captured as a const at module load time in src/services/auth.js:9
  • Test sets process.env.JWT_EXPIRES_IN = '1s' after module load, which has no effect
  • Token is generated with original expiry (likely '1h'), so advancing 2 seconds won't expire it
  • Fix: Use jest.resetModules() and re-import, or refactor generateToken to read the env var dynamically

Non-Blocking Issues

  1. Missing package-lock.json — commit it for reproducible builds
  2. DUMMY_HASH computed at runtime (src/services/auth.js:28) — PR says it's pre-computed but it's hashed via bcrypt.hashSync() at load time
  3. stop() callable multiple times (src/index.js:57-78) — server.close() on already-closed server may throw
  4. Weather service swallows non-404 API errors (src/services/weather.js:53-58) — consider logging errors
  5. No Jest config file — move CLI flags to jest.config.js or package.json
  6. bcrypt.compareSync() blocks event loop — consider async compare() for production

Missing Test Coverage

  • Weather API with API key (mock axios)
  • Weather API non-404 error handling
  • Cache expiration after 10 minutes
  • CORS configuration
  • Error-handling middleware
  • Graceful shutdown (start/stop)
  • Rate limiting (>20 attempts)

github run

@watashikurosaki

Copy link
Copy Markdown

/oc review this pull request

@fnandop

fnandop commented May 4, 2026

Copy link
Copy Markdown
Owner Author

/oc review again please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants