Migrate Forge bridge to @forge/kvs and add /confluence forge reset command#231
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Forge shared-secret reset support, moves bridge storage to ChangesForge reset and bridge registration
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfluenceCommandHandler
participant executeForgeReset
participant ForgeReset
participant ForgeRegister
User->>ConfluenceCommandHandler: forge/reset
ConfluenceCommandHandler->>executeForgeReset: dispatch
executeForgeReset->>ForgeReset: POST signed reset
ForgeReset-->>executeForgeReset: queuedDeleted
executeForgeReset->>ForgeRegister: POST new shared secret
ForgeRegister-->>executeForgeReset: urls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)forge/README.mdmarkdownlint-cli2 wrapper config was not available before execution Comment |
nang2049
added
2: Dev Review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forge/src/index.ts`:
- Around line 179-195: In the wipeAllStorage function, the deletion of
REGISTERED_KEY and SECRET_KEY credentials is currently performed before the
queue deletion loop, which can leave stale queue entries if subsequent
kvs.query() or kvs.delete() operations fail. Reorder the operations by moving
the kvs.delete(REGISTERED_KEY) and kvs.deleteSecret(SECRET_KEY) calls to execute
after the do-while loop that processes queue deletion with the QUEUE_PREFIX,
ensuring credentials remain available for the entire cleanup operation.
In `@server/flow.go`:
- Around line 858-868: The URL assignment block in the flow handling code
accepts bridge-returned values for Reset, Drain, and Register without
validation. Update the logic around the urls handling in flow.go to run each
non-empty value through isForgeWebtriggerURL before writing to
cfg.ForgeResetURL, cfg.ForgeDrainURL, and cfg.ForgeRegisterURL, and only persist
the value when it passes validation.
In `@server/forge_reset.go`:
- Around line 139-144: The code at line 143 (json.Unmarshal call) ignores the
error returned when parsing the response body, and line 144 returns success
without validating that the reset response actually indicates success. Capture
the error from json.Unmarshal instead of ignoring it and check if it's non-nil,
returning an error if the JSON is invalid. Additionally, after successful
unmarshaling, validate that the parsed forgeResetResponse object confirms the
reset was successful (check the appropriate success indicator field in the
forgeResetResponse struct) before returning parsed.QueuedDeleted. Only proceed
with returning success if both the JSON parsing succeeds and the response body
explicitly confirms the bridge wipe was successful.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7841b9ae-0e9b-4ad3-b494-28ca18e09e86
📒 Files selected for processing (10)
forge/README.mdforge/manifest.ymlforge/package.jsonforge/src/index.tsserver/command.goserver/config/main.goserver/flow.goserver/forge_poller.goserver/forge_reset.goserver/forge_reset_test.go
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
LGTM! Left a couple of comments for non-blocking nitpicks
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) |
There was a problem hiding this comment.
Nit: This and server/flow.go could benefit from referencing the same constant for the maximum allowed body size
| case http.StatusOK, http.StatusNoContent: | ||
| return nil | ||
| respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) | ||
| var parsed forgeRegisterResponse |
There was a problem hiding this comment.
nit: Might be nice to also do the same check for parsed.OK that is happening in postForgeReset
2 participants
Summary
@forge/apistoragemodule to@forge/kvsresetwebtrigger to the bridge and awipeRegistrationFnbreak-glass function invocable viaforge invoke./confluence forge resetslash command (System Admin only): rotates the bridge shared secret in-place without re-running the install wizard.QA
/confluence install cloud(capturesForgeResetURL+ForgeRegisterURL)./confluence forge resetas a System Admin.Forge bridge secret rotated successfully. Queued events cleared: N.Non-admin blocked
6. Run
/confluence forge resetas a regular user. Expected:commands can only be run by a system administrator.**HMAC drift **
9. Manually change
ForgeSharedSecretin plugin config to a wrong value.10. Run
/confluence forge reset. Expected: error message mentioningforge invoke -f wipeRegistrationFnandinstall cloud.Break-glass path
11. From a machine with Forge CLI access:
forge invoke -f wipeRegistrationFn -e development.12. Expected: JSON
{"ok":true,"queuedDeleted":N}.13. Re-run
/confluence install cloudto re-register. Verify events flow again.Change Impact: 🔴 High
Reasoning: This PR changes the Forge bridge’s core persistence layer and runtime queue/secrets handling by migrating from
@forge/apistorage to@forge/kvs, and adds new shared-secret rotation/reset flows that can wipe queued events. While there are unit tests for the Go HTTP helpers (postForgeReset/postForgeRegister), there is no end-to-end coverage validating the KVS-backed enqueue→drain→ack and reset/wipe behavior within the Forge webtrigger itself.Regression Risk: High—queue correctness and HMAC/secret coordination across Confluence↔Mattermost depend on KVS secret/key semantics and query/pagination; most critical reset/rotation paths are not directly exercised by unit tests (tests focus on request signing and response parsing rather than full KVS state transitions). The change also expands command routing and inter-service contracts, adding additional runtime failure modes if URLs/secrets drift.
QA Recommendation: Strongly require manual end-to-end QA in staging (do not skip): verify normal enqueue→drain→ack with realistic event volumes; run
/confluence forge resetas a System Admin and confirm the bridge returns an ephemeral confirmation with queued-deleted count and that event flow resumes; verify non-admin access denial; simulate shared-secret/HMAC drift and validate the recovery guidance and break-glassforge invoke -f wipeRegistrationFn ...flow; finally re-register and confirm the queue drains/acknowledges correctly after rotation.Generated by CodeRabbitAI