feat: docs/prds/webhook-trigger.md

[jonit-dev]

Closes #96

Night Watch manages this draft PR automatically so progress is preserved across retries and timeouts.

Status labels:

• nw:partial: implementation is in progress and intentionally incomplete
• nw:resumable: resume this PR before starting new work
• nw:ready-review: implementation is complete and ready for review

[jonit-dev]

🤖 Implemented by Codex

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 88/100

This PR implements a comprehensive webhook trigger system with HMAC authentication, GitHub webhook integration, and UI configuration. The implementation is well-structured with strong test coverage, but has a few notable gaps around timestamp validation and edge case handling.

---

✅ Key Strengths

• Comprehensive Feature Implementation: Full-stack implementation covering config types, server routes, GitHub webhook parsing, and UI settings panel with proper HMAC signature verification.
• Strong Security Foundation: Uses timingSafeEqual for HMAC comparison, validates job IDs against registry, and properly isolates webhook secrets via environment variables.
• Excellent Test Coverage: Both jobs.test.ts and jobs-github.test.ts cover happy paths and error scenarios including signature validation, disallowed jobs, and GitHub event matching.

---

⚠️ Areas for Improvement

• Unused Timestamp Validation: The requireTimestamp and maxSkewSeconds config options are defined and exposed in UI but never implemented in the route handler—either implement or remove these features.
• Missing Error Context: The catch block in job.routes.ts returns generic error messages; consider logging the full error for debugging while still returning safe client messages.
• Type Duplication: IWebhookTriggerConfig and related interfaces are defined in both types.ts and shared/types.ts; consolidate to avoid drift.

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Unused Config Options	job.routes.ts, config.ts	requireTimestamp and maxSkewSeconds are parsed and exposed in UI but never used in route logic.	Medium
Maintainability	Duplicate Type Definitions	types.ts, shared/types.ts	IWebhookTriggerConfig and related interfaces defined in two places could diverge over time.	Low
Security	Raw Body Size Limit	index.ts	1MB limit for raw body may be too large for some webhook payloads; consider if this is appropriate.	Low

---

🔚 Conclusion

Strong implementation ready for merge with one follow-up: either implement timestamp validation or remove the unused config options to avoid user confusion. The core webhook functionality is solid and well-tested.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 82/100

This PR implements a comprehensive webhook trigger feature for Night Watch with solid architecture, good test coverage, and proper security practices. A few implementation gaps and code quality issues prevent a higher score.

---

✅ Key Strengths

• Security-First HMAC Verification: Uses timingSafeEqual for signature comparison and correctly captures raw body before JSON parsing, preventing timing attacks and signature bypass vulnerabilities.
• Comprehensive GitHub Webhook Support: Properly handles multiple GitHub event types (workflow_run, check_suite, pull_request, repository_dispatch) with intelligent rule matching and context extraction.
• Well-Structured Implementation: Clean separation between single-project and global mode routes, reusable handler logic, and comprehensive PRD documentation with clear phases.

---

⚠️ Areas for Improvement

• Unimplemented Timestamp Validation: The requireTimestamp and maxSkewSeconds config fields are defined and validated but never used in the route handler at packages/server/src/routes/job.routes.ts. Requests with stale timestamps would still be accepted when this feature is enabled.
• Duplicate Type Definitions: IWebhookTriggerConfig and related interfaces are defined in both packages/core/src/types.ts and packages/core/src/shared/types.ts, creating potential for divergence and maintenance burden.
• Missing Test Coverage for Edge Cases: No tests for malformed JSON payloads, concurrent dispatch scenarios (lock file conflicts), or when both Night Watch and GitHub signatures are present simultaneously.

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Unimplemented Config Features	packages/server/src/routes/job.routes.ts	requireTimestamp and maxSkewSeconds are validated in config but not enforced in the route.	Medium
Maintainability	Duplicate Type Definitions	packages/core/src/types.ts, shared/types.ts	Identical webhook trigger interfaces defined in two locations could diverge over time.	Low
Testing	Missing Edge Case Tests	packages/server/src/__tests__/server/	No tests for malformed JSON (400), lock conflicts (409), or dual signature scenarios.	Low
Maintainability	Hardcoded vs Dynamic Defaults	templates/night-watch.config.json	Template hardcodes allowedJobIds while code uses getValidJobTypes(), risking inconsistency.	Low

---

🔚 Conclusion

This is a strong feature implementation with proper security practices and good architecture. The unimplemented timestamp validation is the main concern—either implement the feature or remove the config fields. Merge readiness is good after addressing the timestamp validation gap.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 82/100

This PR implements a comprehensive webhook trigger feature with HMAC-signed job dispatch and GitHub webhook integration. The implementation is well-structured with solid test coverage, but has a few maintainability concerns and an incomplete feature.

---

✅ Key Strengths

• Security-First Design: Uses timingSafeEqual for HMAC verification, validates signatures before JSON parsing, and keeps secrets in environment variables rather than config files.
• Comprehensive GitHub Integration: Supports multiple GitHub event types (workflow_run, check_suite, pull_request, repository_dispatch) with flexible rule matching for events, actions, branches, and failure conditions.
• Clean Architecture: Separates concerns well between config normalization, route handlers, and type definitions. Reuses existing spawnAction patterns and lock file mechanisms.

---

⚠️ Areas for Improvement

• Duplicate Type Definitions: IWebhookTriggerConfig, IWebhookTriggerGithubRule, and IWebhookTriggerGithubConfig are defined identically in both packages/core/src/types.ts and packages/core/src/shared/types.ts. Consolidate to a single source and re-export.
• Unimplemented Timestamp Validation: The requireTimestamp and maxSkewSeconds config fields are exposed in the UI and stored but never used in job.routes.ts to validate request timestamps. Either implement the validation or remove the fields.
• Missing Global Mode Test Coverage: The test files cover single-project mode but don't test project-scoped routes (/api/projects/:projectId/jobs/:id/run) which use req.projectConfig and req.projectDir.

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Duplicate Type Definitions	packages/core/src/types.ts, shared/types.ts	Identical interfaces defined in two locations risk divergence during future maintenance.	Medium
Correctness	Unimplemented Timestamp Validation	job.routes.ts	requireTimestamp and maxSkewSeconds config fields are collected but never validated against requests.	Medium
Testing	Missing Global Mode Coverage	jobs.test.ts, jobs-github.test.ts	No tests for project-scoped webhook routes used in global mode.	Low
Maintainability	Branch Glob Semantics	job.routes.ts:globToRegExp	Pattern feat/* matches feat/anything/else/nested since * converts to .*. Consider documenting or using stricter matching.	Low

---

🔚 Conclusion

This is a strong implementation of a complex feature with good security practices and test coverage. The duplicate type definitions should be consolidated before merge, and the timestamp validation should either be implemented or the fields removed to avoid user confusion. Minor test gaps for global mode are non-blocking.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 78/100

This PR implements a comprehensive webhook trigger feature with HMAC-verified job dispatch, GitHub webhook integration, and Settings UI. The implementation is well-structured with good test coverage, but has notable issues with type duplication and an incomplete feature.

---

✅ Key Strengths

• Security-First Design: HMAC signature verification uses timingSafeEqual for constant-time comparison, and job dispatch validates against JOB_REGISTRY to prevent unauthorized execution.
• Comprehensive Configuration: The webhook triggers config includes sensible defaults, validation against the job registry, and support for GitHub event-to-job mapping rules.
• Solid Test Coverage: Tests cover signed dispatch, GitHub webhook parsing, signature validation, and config normalization with good assertions on spawned process environment.

---

⚠️ Areas for Improvement

• Remove Type Duplication: IWebhookTriggerConfig, IWebhookTriggerGithubRule, and IWebhookTriggerGithubConfig are defined in both packages/core/src/types.ts and packages/core/src/shared/types.ts with inconsistent optional fields (webhookTriggers is required in one, optional in the other).
• Implement or Remove Timestamp Validation: The requireTimestamp and maxSkewSeconds config options are defined and documented but never used in the route handler—either implement the feature or remove the dead config.
• Extract Shared Clone Helper: The cloneWebhookTriggers function is duplicated in both packages/core/src/config.ts and packages/cli/src/commands/init.ts.

---

🐛 Bugs Found

Bug Name	Affected Files	Description	Confidence
Undefined Function Reference	packages/core/src/constants.ts	getValidJobTypes() is called in DEFAULT_WEBHOOK_TRIGGERS.allowedJobIds but is not shown as imported in the diff. If this function doesn't exist in the file, this will throw ReferenceError at module load time.	Medium 🟡

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Type Definition Duplication	packages/core/src/types.ts, packages/core/src/shared/types.ts	Identical webhook trigger types defined in two locations with different optionality, risking confusion and sync issues.	Medium
Testing	Missing Timestamp Validation Tests	packages/server/src/__tests__/server/jobs.test.ts	The requireTimestamp feature has no test coverage, and the feature itself is unimplemented in the handler.	Low
Testing	Missing Multi-Signature Header Test	packages/server/src/routes/job.routes.ts	No test case for when both X-Night-Watch-Signature and X-Hub-Signature-256 headers are present.	Low

---

🔚 Conclusion

This is a solid implementation of a valuable feature with good security practices and documentation. The type duplication and undefined function reference should be resolved before merge; the timestamp validation feature should either be implemented or removed from the config schema.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 82/100

This PR implements a comprehensive webhook trigger feature with HMAC-signed job dispatch and GitHub webhook integration. The implementation is well-structured with solid security practices, but has a few notable gaps around duplicate type definitions and an unimplemented config feature.

---

✅ Key Strengths

• Security-First Design: Uses timingSafeEqual for HMAC comparison, validates signatures before JSON parsing, and properly checks job allowlists before dispatch.
• Comprehensive GitHub Integration: Supports multiple GitHub event types (workflow_run, check_suite, pull_request, repository_dispatch) with flexible rule matching for events, actions, branches, and failure conditions.
• Thorough Test Coverage: Tests cover unsigned requests, valid signatures, disallowed jobs, GitHub webhook scenarios, and config validation edge cases.

---

⚠️ Areas for Improvement

• Duplicate Type Definitions: IWebhookTriggerConfig, IWebhookTriggerGithubRule, and IWebhookTriggerGithubConfig are defined identically in both packages/core/src/types.ts and packages/core/src/shared/types.ts. This should be consolidated to a single source of truth to prevent drift.
• Unimplemented requireTimestamp Feature: The config includes requireTimestamp and maxSkewSeconds fields, but the route handler in job.routes.ts never validates timestamps. This is dead config that will confuse users who enable it expecting protection against replay attacks.
• Fragile Middleware Order: Raw body parsing in setupJobRawBodyParsing() must run before express.json(). While currently correct, a future refactor could easily break signature verification. Consider adding a comment or defensive check.

---

🐛 Bugs Found

Bug Name	Affected Files	Description	Confidence
Dead Config: Timestamp Validation	packages/server/src/routes/job.routes.ts, packages/core/src/types.ts	requireTimestamp and maxSkewSeconds are accepted in config but never used in the dispatch handler, misleading users who expect replay attack protection.	High 🟢

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Maintainability	Duplicate Type Definitions	packages/core/src/types.ts, packages/core/src/shared/types.ts	Identical webhook trigger interfaces defined in two locations risk divergence and increase maintenance burden.	Medium
Maintainability	Missing Exhaustive Return	packages/server/src/routes/job.routes.ts	getLockPathForJob() has no fallback return; adding a new JobType without updating the switch would cause runtime errors.	Low

---

🔚 Conclusion

This is a well-executed feature with strong security fundamentals and good test coverage. The duplicate types and unimplemented timestamp validation should be resolved before merge to avoid user confusion and technical debt. The core functionality is production-ready.

---

Analyzed using z-ai/glm-5

[github-actions]

DiffGuard AI Analysis

AI Review Summary

🏆 Overall Score: 82/100

This PR implements a comprehensive inbound webhook trigger system with HMAC authentication, GitHub webhook support, and a settings UI. The implementation is well-structured with good test coverage, but has one notable incomplete feature (timestamp validation) and a few maintainability concerns.

---

✅ Key Strengths

• Security-First Design: HMAC signature verification uses timingSafeEqual to prevent timing attacks, validates signature format before computing HMAC, and parses JSON only after signature verification—proper security ordering throughout.
• Comprehensive GitHub Integration: Supports four GitHub event types with proper header parsing, branch pattern matching via glob-to-regex conversion, and contextual environment variables passed to spawned jobs.
• Clean Architecture: Reuses existing spawnAction patterns, properly integrates with the job registry, and maintains consistency with the existing action route conventions for both single-project and global modes.

---

⚠️ Areas for Improvement

• Incomplete Timestamp Validation: The requireTimestamp and maxSkewSeconds config fields are defined, validated, and exposed in the UI, but the server route (job.routes.ts) contains no implementation to check timestamps. This creates dead configuration that users will expect to work.
• Missing Exhaustive Job Type Handling: The getLockPathForJob function lacks a default case or TypeScript exhaustive check. Adding a new JobType to the registry without updating this function would result in undefined lock paths and runtime errors.
• Type Definition Duplication: IWebhookTriggerConfig and related interfaces are defined in both packages/core/src/types.ts and packages/core/src/shared/types.ts, creating potential for drift and confusion about which source of truth to maintain.

---

📋 Issues Found

Issue Type	Issue Name	Affected Components	Description	Impact/Severity
Correctness	Missing Timestamp Validation	packages/server/src/routes/job.routes.ts	requireTimestamp and maxSkewSeconds are configured but never used to validate request timestamps.	Medium
Maintainability	Missing Default Case in Switch	packages/server/src/routes/job.routes.ts	getLockPathForJob switch statement returns undefined for unhandled job types, which could cause runtime failures.	Low
Maintainability	Duplicate Type Definitions	packages/core/src/types.ts, packages/core/src/shared/types.ts	Same interfaces defined in two places; web/api.ts imports from one while core uses the other.	Low
Testing	Missing Global Mode Tests	packages/server/src/__tests__/server/jobs.test.ts	Tests only cover single-project mode; project-scoped routes (/api/projects/:projectId/jobs/:id/run) are untested.	Low

---

🔚 Conclusion

This is a well-executed feature with solid fundamentals, but the timestamp validation gap should be addressed before merge—either by implementing the validation or removing the dead config fields. The maintainability issues are non-blocking but worth tracking for future cleanup.

---

Analyzed using z-ai/glm-5

[jonit-dev]

✅ Ready for Human Review

Night Watch reviewed this PR (score: 78/100) at commit 968be088d72b and found no automated fixes to apply for the current head.

This PR is ready for human review and merge.