Plugins/real telegram workflow - #5
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request transforms FlowForge from a demo CLI into a production-ready system with persistent storage, HTTP API, and command-line tooling. It introduces workflow and secret stores (memory and file-based), a full REST API with handlers for resource CRUD and Telegram webhooks, live Telegram message sending with proxy support, background polling for incoming messages, server initialization with encrypted storage, and a command-line client with shell completions. ChangesServer & CLI Implementation
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
internal/plugins/telegram/send.go (1)
162-172: ⚡ Quick winHTTP client recreation shares a common root cause. Both
SendOperation.sendMessageandPoller.pollOncecreate a new HTTP client on every invocation whenHTTPClientis nil. The same caching pattern (store after first creation) applies to both sites. Consider a unified approach: either cache on the respective struct, or extract client initialization to a shared helper that both can call once during setup.🤖 Prompt for 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. In `@internal/plugins/telegram/send.go` around lines 162 - 172, SendOperation.sendMessage and Poller.pollOnce both recreate an HTTP client when the struct's HTTPClient is nil; unify client initialization to avoid repeated creation by either caching the created client on the owning struct (set s.HTTPClient after successful newHTTPClient(proxy)) or extracting initialization into a shared helper (e.g., initHTTPClient(ctx) that calls proxyURL(ctx) and newHTTPClient(proxy) once) and have both sendMessage and pollOnce call that helper during setup; reference HTTPClient, SendOperation.sendMessage, Poller.pollOnce, proxyURL(ctx), and newHTTPClient(proxy) when applying the change.internal/api/telegram_webhook.go (1)
19-37: Refactor duplicated Telegram DTOs andtelegramUpdateEvent
ThetelegramUpdate,telegramMessage,telegramUser,telegramChattypes and thetelegramUpdateEventfunction are duplicated betweeninternal/api/telegram_webhook.goandinternal/plugins/telegram/poller.go, creating drift risk. Extract the shared DTOs + event conversion into a common internaltelegrampackage/file (e.g.,internal/plugins/telegram/types.go) and reuse from both call sites.🤖 Prompt for 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. In `@internal/api/telegram_webhook.go` around lines 19 - 37, The Telegram DTOs (telegramUpdate, telegramMessage, telegramUser, telegramChat) and the telegramUpdateEvent conversion are duplicated; extract these symbols into a single shared internal package/file (create a shared telegram types module), move the definitions of telegramUpdate, telegramMessage, telegramUser, telegramChat and the telegramUpdateEvent function there, update the webhook and poller code to import and use the shared types and function instead of their local copies, remove the duplicates, and run/update any imports or references accordingly.internal/api/secrets_test.go (1)
14-41: ⚡ Quick winUse
httptest.NewRequestWithContextto satisfy thenoctxlinter.The linter correctly flags that
httptest.NewRequestshould be replaced withhttptest.NewRequestWithContext.🔧 Proposed fix
- handler.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/v1/secrets", nil)) + handler.ServeHTTP(response, httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/v1/secrets", nil))The
contextimport is already present at line 4.🤖 Prompt for 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. In `@internal/api/secrets_test.go` around lines 14 - 41, In TestListSecretsRedactsValues, replace the use of httptest.NewRequest with httptest.NewRequestWithContext and pass a context (e.g., context.Background()) so the test satisfies the noctx linter; update the call that constructs the request used in handler.ServeHTTP inside TestListSecretsRedactsValues to use httptest.NewRequestWithContext (the context import is already present), leaving the rest of the test unchanged.Source: Linters/SAST tools
internal/api/workflows_test.go (1)
32-32: ⚡ Quick winUse
httptest.NewRequestWithContextto satisfy thenoctxlinter.The linter correctly flags that
httptest.NewRequestshould be replaced withhttptest.NewRequestWithContextto ensure request contexts are properly initialized.🔧 Proposed fix
Line 32:
- server.ServeHTTP(response, httptest.NewRequest(http.MethodGet, "/v1/workflows/notify", nil)) + server.ServeHTTP(response, httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/v1/workflows/notify", nil))Line 67:
- request := httptest.NewRequest(method, path, &requestBody) + request := httptest.NewRequestWithContext(context.Background(), method, path, &requestBody)Add
contextto the imports:import ( "bytes" + "context" "encoding/json"Also applies to: 67-67
🤖 Prompt for 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. In `@internal/api/workflows_test.go` at line 32, Replace usages of httptest.NewRequest with httptest.NewRequestWithContext to satisfy the noctx linter: update the calls passed into server.ServeHTTP (e.g., the one using http.MethodGet and path "/v1/workflows/notify" and the other invocation around line 67) to use context.Background() (or a test ctx) as the first argument, and add "context" to the test imports; ensure the HTTP method and URL (http.MethodGet, "/v1/workflows/notify") remain unchanged when calling NewRequestWithContext.Source: Linters/SAST tools
cmd/server/main.go (1)
152-171: 💤 Low valueConsider logging a warning when generating a new master key.
The function silently generates and persists a new master key on first run. While the documentation mentions backing up the key, users may not realize a critical key was created until secrets are already encrypted. A startup log warning would improve operational awareness.
📝 Proposed enhancement
key, err = secrets.GenerateMasterKey() if err != nil { return nil, err } + log.Printf("WARNING: Generated new master key at %s - back up this file before encrypting production secrets", path) if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { return nil, err }🤖 Prompt for 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. In `@cmd/server/main.go` around lines 152 - 171, The function loadOrCreateMasterKey silently generates and writes a new key when the file does not exist; update it to emit a startup warning when creating a new master key: detect the code path where os.IsNotExist(err) leads to secrets.GenerateMasterKey(), and before or immediately after os.WriteFile(...) call a logger (e.g., app logger or standard log) to warn that a new master key was generated and persisted at the given path and must be backed up; ensure the log includes the path and backup instruction but never prints the key bytes themselves.internal/secrets/file_store.go (1)
100-113: 💤 Low valueConsider documenting
ReplacevsUpdatesemantics.
Replacebypasses immutability checks (usesValidateCreaterather thanValidateUpdate), allowing replacement of immutable secrets for rotation scenarios.Update(line 80) honors immutability. A brief comment distinguishing the two would help future maintainers understand when to use each.📝 Suggested comment
+// Replace unconditionally replaces an existing secret, bypassing immutability checks. +// Use Replace for secret rotation; use Update for normal updates that honor immutability. func (s *EncryptedFileStore) Replace(_ context.Context, secret SecretResource) error {🤖 Prompt for 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. In `@internal/secrets/file_store.go` around lines 100 - 113, Add a brief comment above the EncryptedFileStore.Replace method explaining that Replace intentionally uses ValidateCreate (not ValidateUpdate) and therefore bypasses immutability checks to allow secret rotation/replacement, whereas the Update method uses ValidateUpdate and enforces immutability; reference the Replace function name, the Update method, and the ValidateCreate/ValidateUpdate helpers so maintainers know the semantic difference and when to use each.
🤖 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 `@cmd/flowforge/completion.go`:
- Around line 29-33: The fish completion branches in completion.go only check
singular resource kinds, so update the __fish_seen_subcommand_from conditionals
used in the complete invocations (the lines that currently test for 'get',
'delete', and the secret/workflow checks) to also accept plural forms (e.g., add
'secrets', 'workflows' alongside 'secret' and 'workflow') so completion triggers
for both singular and plural CLI usages; locate the complete calls around the
existing '__fish_seen_subcommand_from get/delete' checks and add the plural
variants in the conditional lists for resource-kind and name completions (also
apply the same fix to the other occurrences noted at the file regions
referenced).
In `@cmd/flowforge/main.go`:
- Line 174: The apply/update/delete URL construction is appending raw resource
names (strings.TrimRight(*server, "/")+endpoint+"/"+name) which can break when
names contain reserved characters; replace the raw name with
url.PathEscape(name) so the path is encoded (e.g., strings.TrimRight(*server,
"/")+endpoint+"/"+url.PathEscape(name)); update the three occurrences mentioned
(the current line and the ones at lines ~186 and ~210) and ensure net/url is
imported if not already; note runGet already uses url.PathEscape as a reference
for the correct behavior.
- Around line 263-267: The delete command currently uses
isManifestPath(positionals[1]) and treats any name ending in .yaml/.json as a
manifest path, which misclassifies resource names like "foo.yaml"; update the
logic so the code only treats the argument as a manifest path when the file
actually exists (e.g. use os.Stat or equivalent file-existence check) before
calling buildApplyRequest(positionals[1]); adjust isManifestPath or the
conditional in the delete handling to require existence, keeping references to
isManifestPath and buildApplyRequest (and the delete <kind> <name> handling) so
resource names with .yaml/.json are not wrongly interpreted as files.
- Line 20: The CLI uses http.DefaultClient and http.NewRequest so requests can
hang indefinitely; change resourceHTTPClient to a client with a sensible timeout
(e.g. &http.Client{Timeout: 30*time.Second}) and make sendResourceRequest
context-aware by accepting a context.Context and creating requests with
http.NewRequestWithContext (or attaching ctx via req = req.WithContext(ctx))
before calling resourceHTTPClient.Do; update callers to pass a context (e.g. ctx
from main) so requests are both timeout-bound and cancelable.
In `@cmd/server/main.go`:
- Around line 108-127: dispatchEvent currently stops on the first failing
workflow run; change it to attempt runs.Run for every matching workflow from
workflows.List and collect errors instead of returning immediately. Inside
dispatchEvent iterate through matching workflows, call runs.Run(ctx,
kernel.RunRequest{...}) for each, and if it returns an error append a contextual
error (including workflow.Metadata.Name) to an error accumulator (e.g., slice of
errors or a multierror) and continue; after the loop, return nil if no errors
collected or return an aggregated error (joined or wrapped) that includes all
individual run errors so all matching workflows are attempted and failures are
reported together.
- Around line 103-106: Replace the direct call to http.ListenAndServe with an
http.Server instance that sets ReadTimeout, WriteTimeout and IdleTimeout to
reasonable values (e.g., a few seconds for read/write and a minute for idle);
construct the server with Addr: addr and Handler: server.Handler(), log the same
info, then call server.ListenAndServe() on that instance. Also add the time
import so you can specify durations. Ensure you reference the existing
server.Handler() and the previous ListenAndServe usage when making the change.
In `@examples/plugins/telegram.yaml`:
- Line 10: Update the proxy secret reference in examples/plugins/telegram.yaml
so it uses the same full relative path as the bot secret; replace the bare
"telegram-proxy.yaml" reference with "examples/secrets/telegram-proxy.yaml" to
prevent copy/paste errors and keep both secret paths consistent.
In `@internal/api/server.go`:
- Around line 77-79: writeError currently returns err.Error() for all statuses
which may leak sensitive internal details for 5xx responses; update writeError
to avoid exposing internal errors to clients by returning the actual error
message only for client errors (status < 500) and returning a generic message
like "internal server error" for server errors (status >= 500), ensure you still
log the original error internally (e.g., via an existing logger) for debugging,
and keep using writeJSON to emit the chosen message; also guard against nil err
before calling Error() and reference the writeError function and writeJSON in
your changes.
- Around line 58-60: The current decode only parses the first JSON value and
silently allows trailing JSON; after creating the decoder (json.NewDecoder) and
calling DisallowUnknownFields(), keep the existing Decode(out) call but then
attempt a second Decode into a dummy variable (e.g., var extra interface{}) and
check for io.EOF — if the second Decode returns nil or any non-EOF error, return
an error indicating unexpected trailing JSON; reference the decoder variable,
json.NewDecoder, DisallowUnknownFields, and the initial Decode(out) so you add
this trailing-data check immediately after the existing Decode call.
- Around line 52-60: The decodeResource function currently reads an unbounded
request body; change its signature to accept the ResponseWriter (e.g.,
decodeResource(w http.ResponseWriter, r *http.Request, out any)) and wrap r.Body
with http.MaxBytesReader(w, r.Body, MaxRequestBodyBytes) (define a
MaxRequestBodyBytes constant) before creating the json.Decoder; keep
decoder.DisallowUnknownFields(), handle the returned error from decoder.Decode
to translate an http.ErrBodyTooLarge/“request body too large” case into an
appropriate error response (or a custom error type) and ensure r.Body is closed
as before; update all call sites to pass the ResponseWriter to decodeResource.
In `@internal/plugins/telegram/send_test.go`:
- Line 251: Replace the call to http.NewRequest in the test with the
context-aware variant http.NewRequestWithContext: update the code that
constructs the request (the line creating req) to pass a context (use the
existing ctx if available or context.Background() in the test) so the request is
created with a context; keep the rest of the test logic identical and ensure err
handling around the request construction remains unchanged.
In `@internal/plugins/telegram/send.go`:
- Around line 262-264: The type assertion
http.DefaultTransport.(*http.Transport) in newHTTPClient can panic if
DefaultTransport is not an *http.Transport; change it to a safe type-assertion
with the comma-ok idiom, e.g., attempt to cast into *http.Transport and if that
fails create a sensible default transport (or return an error) before calling
Clone(); ensure the variable transport is always a valid *http.Transport (either
the cloned one or a newly constructed &http.Transport{}) so downstream proxy
logic can safely use it.
In `@internal/store/file_workflow_store.go`:
- Around line 34-41: Get in FileWorkflowStore returns the unmarshaled
kernel.WorkflowResource from read() directly, allowing callers to mutate
internal data; modify FileWorkflowStore.Get to return a defensive deep copy
(reuse the same deepCopy() used by MemoryWorkflowStore.Get or create one
matching secrets store behavior) so callers receive a copy, not the original.
Locate FileWorkflowStore.Get and the read() method, call deepCopy() on the
returned kernel.WorkflowResource before returning (or implement an equivalent
deep copy helper), and ensure locking semantics remain unchanged.
In `@internal/store/memory_workflow_store.go`:
- Around line 66-78: MemoryWorkflowStore.List currently appends the stored
WorkflowResource values directly from s.workflows into out, which can leak
internal mutable state if WorkflowResource contains reference types; change the
loop in List to append deep copies instead of original references by creating a
copy for each workflow (e.g., invoke an existing Clone/DeepCopy method on
kernel.WorkflowResource or implement one) and append that copy to out; keep the
same sort on out[i].Metadata.Name and return the copied slice.
- Around line 29-37: MemoryWorkflowStore currently returns and stores
kernel.WorkflowResource by reference, allowing callers to mutate internal
maps/slices; add a deepCopy function for kernel.WorkflowResource that
recursively clones Metadata.Labels, Metadata.Annotations, TriggerDef.With, each
StepDef.With, StepDef.Then/Else slices and any nested maps/slices, then use this
deepCopy in MemoryWorkflowStore.Get and MemoryWorkflowStore.List to return
copies and also use it on write paths (MemoryWorkflowStore.Create, Save, Update)
to store a cloned instance so the internal map never shares backing references
with callers.
---
Nitpick comments:
In `@cmd/server/main.go`:
- Around line 152-171: The function loadOrCreateMasterKey silently generates and
writes a new key when the file does not exist; update it to emit a startup
warning when creating a new master key: detect the code path where
os.IsNotExist(err) leads to secrets.GenerateMasterKey(), and before or
immediately after os.WriteFile(...) call a logger (e.g., app logger or standard
log) to warn that a new master key was generated and persisted at the given path
and must be backed up; ensure the log includes the path and backup instruction
but never prints the key bytes themselves.
In `@internal/api/secrets_test.go`:
- Around line 14-41: In TestListSecretsRedactsValues, replace the use of
httptest.NewRequest with httptest.NewRequestWithContext and pass a context
(e.g., context.Background()) so the test satisfies the noctx linter; update the
call that constructs the request used in handler.ServeHTTP inside
TestListSecretsRedactsValues to use httptest.NewRequestWithContext (the context
import is already present), leaving the rest of the test unchanged.
In `@internal/api/telegram_webhook.go`:
- Around line 19-37: The Telegram DTOs (telegramUpdate, telegramMessage,
telegramUser, telegramChat) and the telegramUpdateEvent conversion are
duplicated; extract these symbols into a single shared internal package/file
(create a shared telegram types module), move the definitions of telegramUpdate,
telegramMessage, telegramUser, telegramChat and the telegramUpdateEvent function
there, update the webhook and poller code to import and use the shared types and
function instead of their local copies, remove the duplicates, and run/update
any imports or references accordingly.
In `@internal/api/workflows_test.go`:
- Line 32: Replace usages of httptest.NewRequest with
httptest.NewRequestWithContext to satisfy the noctx linter: update the calls
passed into server.ServeHTTP (e.g., the one using http.MethodGet and path
"/v1/workflows/notify" and the other invocation around line 67) to use
context.Background() (or a test ctx) as the first argument, and add "context" to
the test imports; ensure the HTTP method and URL (http.MethodGet,
"/v1/workflows/notify") remain unchanged when calling NewRequestWithContext.
In `@internal/plugins/telegram/send.go`:
- Around line 162-172: SendOperation.sendMessage and Poller.pollOnce both
recreate an HTTP client when the struct's HTTPClient is nil; unify client
initialization to avoid repeated creation by either caching the created client
on the owning struct (set s.HTTPClient after successful newHTTPClient(proxy)) or
extracting initialization into a shared helper (e.g., initHTTPClient(ctx) that
calls proxyURL(ctx) and newHTTPClient(proxy) once) and have both sendMessage and
pollOnce call that helper during setup; reference HTTPClient,
SendOperation.sendMessage, Poller.pollOnce, proxyURL(ctx), and
newHTTPClient(proxy) when applying the change.
In `@internal/secrets/file_store.go`:
- Around line 100-113: Add a brief comment above the EncryptedFileStore.Replace
method explaining that Replace intentionally uses ValidateCreate (not
ValidateUpdate) and therefore bypasses immutability checks to allow secret
rotation/replacement, whereas the Update method uses ValidateUpdate and enforces
immutability; reference the Replace function name, the Update method, and the
ValidateCreate/ValidateUpdate helpers so maintainers know the semantic
difference and when to use each.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4487dbcd-bfbd-4d45-9b66-2f1bdaa89d3a
📒 Files selected for processing (32)
README.mdcmd/flowforge/completion.gocmd/flowforge/main.gocmd/flowforge/main_test.gocmd/server/main.godocs/development.mddocs/plugins.mddocs/secrets.mdexamples/plugins/ai.yamlexamples/plugins/storage.yamlexamples/plugins/telegram.yamlexamples/secrets/telegram-bot.yamlexamples/secrets/telegram-proxy.yamlinternal/api/runs.gointernal/api/secrets_test.gointernal/api/server.gointernal/api/telegram_webhook.gointernal/api/workflows.gointernal/api/workflows_test.gointernal/plugins/telegram/manifest.gointernal/plugins/telegram/poller.gointernal/plugins/telegram/send.gointernal/plugins/telegram/send_test.gointernal/secrets/file_store.gointernal/secrets/file_store_test.gointernal/secrets/store.gointernal/secrets/store_test.gointernal/store/file_workflow_store.gointernal/store/file_workflow_store_test.gointernal/store/memory_workflow_store.gointernal/store/memory_workflow_store_test.gointernal/store/workflow_store.go
Comment on lines
+29
to
+33
| complete -c flowforge -n '__fish_seen_subcommand_from get; and __fish_seen_subcommand_from secret workflow' -a '(__flowforge_resource_names)' -d 'Resource name' | ||
| complete -c flowforge -n '__fish_seen_subcommand_from delete; and not __fish_seen_subcommand_from secret secrets workflow workflows' -a 'secret workflow' -d 'Resource kind' | ||
| complete -c flowforge -n '__fish_seen_subcommand_from delete' -s f -d 'Resource manifest' -r -F | ||
| complete -c flowforge -n '__fish_seen_subcommand_from delete' -l server -d 'Server base URL' -r | ||
| complete -c flowforge -n '__fish_seen_subcommand_from delete; and __fish_seen_subcommand_from secret workflow' -a '(__flowforge_resource_names)' -d 'Resource name' |
Contributor
There was a problem hiding this comment.
Completion kind vocabulary is out of sync with CLI parsing (plural forms).
CLI parsing accepts both singular and plural kinds, but these completion branches only wire singular delete/name flows. This causes missing suggestions for valid commands (e.g., get secrets, delete workflows paths).
Also applies to: 68-69, 113-114
🤖 Prompt for 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.
In `@cmd/flowforge/completion.go` around lines 29 - 33, The fish completion
branches in completion.go only check singular resource kinds, so update the
__fish_seen_subcommand_from conditionals used in the complete invocations (the
lines that currently test for 'get', 'delete', and the secret/workflow checks)
to also accept plural forms (e.g., add 'secrets', 'workflows' alongside 'secret'
and 'workflow') so completion triggers for both singular and plural CLI usages;
locate the complete calls around the existing '__fish_seen_subcommand_from
get/delete' checks and add the plural variants in the conditional lists for
resource-kind and name completions (also apply the same fix to the other
occurrences noted at the file regions referenced).
|
|
||
| const defaultServer = "http://127.0.0.1:8080" | ||
|
|
||
| var resourceHTTPClient = http.DefaultClient |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bhttp\.NewRequest\('
rg -nP 'resourceHTTPClient\s*=\s*http\.DefaultClient'Repository: rezaqomy/FlowForge
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- main.go top (1-80) ----"
sed -n '1,80p' cmd/flowforge/main.go
echo "---- main.go around 320-380 ----"
sed -n '300,380p' cmd/flowforge/main.go
echo "---- resourceHTTPClient usages ----"
rg -n 'resourceHTTPClient' cmd/flowforge/main.goRepository: rezaqomy/FlowForge
Length of output: 4295
Add timeout-bound, context-aware HTTP requests to the CLI
cmd/flowforge/main.go sets resourceHTTPClient to http.DefaultClient (no timeout) and sendResourceRequest creates requests with http.NewRequest (no context) before calling resourceHTTPClient.Do(req), so a stalled server can hang the CLI indefinitely.
Suggested fix
import (
+ "context"
"bytes"
"encoding/json"
"flag"
"fmt"
"io"
"net/http"
"net/url"
"os"
"path/filepath"
"strings"
+ "time"
"gopkg.in/yaml.v3"
)
@@
-var resourceHTTPClient = http.DefaultClient
+var resourceHTTPClient = &http.Client{Timeout: 15 * time.Second}
@@
func sendResourceRequest(method, url string, body []byte) (int, []byte, error) {
- req, err := http.NewRequest(method, url, bytes.NewReader(body))
+ ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+ defer cancel()
+ req, err := http.NewRequestWithContext(ctx, method, url, bytes.NewReader(body))
if err != nil {
return 0, nil, err
}🤖 Prompt for 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.
In `@cmd/flowforge/main.go` at line 20, The CLI uses http.DefaultClient and
http.NewRequest so requests can hang indefinitely; change resourceHTTPClient to
a client with a sensible timeout (e.g. &http.Client{Timeout: 30*time.Second})
and make sendResourceRequest context-aware by accepting a context.Context and
creating requests with http.NewRequestWithContext (or attaching ctx via req =
req.WithContext(ctx)) before calling resourceHTTPClient.Do; update callers to
pass a context (e.g. ctx from main) so requests are both timeout-bound and
cancelable.
Source: Linters/SAST tools
| if status == http.StatusConflict { | ||
| status, responseBody, err = sendResourceRequest( | ||
| http.MethodPut, | ||
| strings.TrimRight(*server, "/")+endpoint+"/"+name, |
Contributor
There was a problem hiding this comment.
Escape resource names when constructing apply/delete URLs.
runGet already uses url.PathEscape (Line 68), but apply/update and delete paths append raw names. Names containing reserved characters can produce wrong request targets.
Suggested fix
- strings.TrimRight(*server, "/")+endpoint+"/"+name,
+ strings.TrimRight(*server, "/")+endpoint+"/"+url.PathEscape(name),
@@
- strings.TrimRight(*server, "/")+endpoint+"/"+name+"?replace=true",
+ strings.TrimRight(*server, "/")+endpoint+"/"+url.PathEscape(name)+"?replace=true",
@@
- url := strings.TrimRight(server, "/") + endpoint + "/" + name
+ url := strings.TrimRight(server, "/") + endpoint + "/" + url.PathEscape(name)Also applies to: 186-186, 210-210
🤖 Prompt for 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.
In `@cmd/flowforge/main.go` at line 174, The apply/update/delete URL construction
is appending raw resource names (strings.TrimRight(*server,
"/")+endpoint+"/"+name) which can break when names contain reserved characters;
replace the raw name with url.PathEscape(name) so the path is encoded (e.g.,
strings.TrimRight(*server, "/")+endpoint+"/"+url.PathEscape(name)); update the
three occurrences mentioned (the current line and the ones at lines ~186 and
~210) and ensure net/url is imported if not already; note runGet already uses
url.PathEscape as a reference for the correct behavior.
Comment on lines
+263
to
+267
| if isManifestPath(positionals[1]) { | ||
| _, _, header, name, err := buildApplyRequest(positionals[1]) | ||
| if err != nil { | ||
| return "", "", "", err | ||
| } |
Contributor
There was a problem hiding this comment.
delete <kind> <name> misclassifies names ending with .yaml/.json as manifest paths.
Line 263 treats any such name as a file path, so valid resource names like foo.yaml cannot be deleted unless a matching file exists.
Suggested fix
- if isManifestPath(positionals[1]) {
+ if isManifestPath(positionals[1]) {
+ if stat, statErr := os.Stat(positionals[1]); statErr != nil || stat.IsDir() {
+ return kind, positionals[1], server, nil
+ }
_, _, header, name, err := buildApplyRequest(positionals[1])
if err != nil {
return "", "", "", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if isManifestPath(positionals[1]) { | |
| _, _, header, name, err := buildApplyRequest(positionals[1]) | |
| if err != nil { | |
| return "", "", "", err | |
| } | |
| if isManifestPath(positionals[1]) { | |
| if stat, statErr := os.Stat(positionals[1]); statErr != nil || stat.IsDir() { | |
| return kind, positionals[1], server, nil | |
| } | |
| _, _, header, name, err := buildApplyRequest(positionals[1]) | |
| if err != nil { | |
| return "", "", "", err | |
| } |
🤖 Prompt for 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.
In `@cmd/flowforge/main.go` around lines 263 - 267, The delete command currently
uses isManifestPath(positionals[1]) and treats any name ending in .yaml/.json as
a manifest path, which misclassifies resource names like "foo.yaml"; update the
logic so the code only treats the argument as a manifest path when the file
actually exists (e.g. use os.Stat or equivalent file-existence check) before
calling buildApplyRequest(positionals[1]); adjust isManifestPath or the
conditional in the delete handling to require existence, keeping references to
isManifestPath and buildApplyRequest (and the delete <kind> <name> handling) so
resource names with .yaml/.json are not wrongly interpreted as files.
Comment on lines
+103
to
+106
| log.Printf("flowforge server listening on %s", addr) | ||
| log.Printf("state directory: %s", dataDir) | ||
| return http.ListenAndServe(addr, server.Handler()) | ||
| } |
Contributor
There was a problem hiding this comment.
Configure HTTP server timeouts to prevent resource exhaustion.
http.ListenAndServe does not set read, write, or idle timeouts, leaving the server vulnerable to slowloris attacks and resource exhaustion from hanging connections.
🔒 Proposed fix to add timeouts
log.Printf("flowforge server listening on %s", addr)
log.Printf("state directory: %s", dataDir)
- return http.ListenAndServe(addr, server.Handler())
+ srv := &http.Server{
+ Addr: addr,
+ Handler: server.Handler(),
+ ReadTimeout: 15 * time.Second,
+ WriteTimeout: 15 * time.Second,
+ IdleTimeout: 60 * time.Second,
+ }
+ return srv.ListenAndServe()Add time to the imports:
"os"
"path/filepath"
"strings"
+ "time"🤖 Prompt for 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.
In `@cmd/server/main.go` around lines 103 - 106, Replace the direct call to
http.ListenAndServe with an http.Server instance that sets ReadTimeout,
WriteTimeout and IdleTimeout to reasonable values (e.g., a few seconds for
read/write and a minute for idle); construct the server with Addr: addr and
Handler: server.Handler(), log the same info, then call server.ListenAndServe()
on that instance. Also add the time import so you can specify durations. Ensure
you reference the existing server.Handler() and the previous ListenAndServe
usage when making the change.
| if !ok { | ||
| t.Fatalf("transport type = %T, want *http.Transport", client.Transport) | ||
| } | ||
| req, err := http.NewRequest(http.MethodPost, "https://api.telegram.org/bottest/sendMessage", nil) |
Contributor
There was a problem hiding this comment.
Use http.NewRequestWithContext instead of http.NewRequest.
Static analysis correctly identifies this should use the context-aware variant for consistency and to avoid potential linter failures in CI.
🔧 Suggested fix
- req, err := http.NewRequest(http.MethodPost, "https://api.telegram.org/bottest/sendMessage", nil)
+ req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://api.telegram.org/bottest/sendMessage", nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| req, err := http.NewRequest(http.MethodPost, "https://api.telegram.org/bottest/sendMessage", nil) | |
| req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://api.telegram.org/bottest/sendMessage", nil) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 251-251: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
🤖 Prompt for 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.
In `@internal/plugins/telegram/send_test.go` at line 251, Replace the call to
http.NewRequest in the test with the context-aware variant
http.NewRequestWithContext: update the code that constructs the request (the
line creating req) to pass a context (use the existing ctx if available or
context.Background() in the test) so the request is created with a context; keep
the rest of the test logic identical and ensure err handling around the request
construction remains unchanged.
Source: Linters/SAST tools
Comment on lines
+262
to
+264
| func newHTTPClient(proxy string) (*http.Client, error) { | ||
| transport := http.DefaultTransport.(*http.Transport).Clone() | ||
| if proxy != "" { |
Contributor
There was a problem hiding this comment.
Type assertion on http.DefaultTransport may panic.
If http.DefaultTransport has been replaced with a non-*http.Transport type (rare but possible in test harnesses or custom runtimes), this assertion will panic.
🛡️ Defensive fix
func newHTTPClient(proxy string) (*http.Client, error) {
- transport := http.DefaultTransport.(*http.Transport).Clone()
+ defaultTransport, ok := http.DefaultTransport.(*http.Transport)
+ if !ok {
+ defaultTransport = &http.Transport{}
+ }
+ transport := defaultTransport.Clone()
if proxy != "" {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func newHTTPClient(proxy string) (*http.Client, error) { | |
| transport := http.DefaultTransport.(*http.Transport).Clone() | |
| if proxy != "" { | |
| func newHTTPClient(proxy string) (*http.Client, error) { | |
| defaultTransport, ok := http.DefaultTransport.(*http.Transport) | |
| if !ok { | |
| defaultTransport = &http.Transport{} | |
| } | |
| transport := defaultTransport.Clone() | |
| if proxy != "" { |
🤖 Prompt for 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.
In `@internal/plugins/telegram/send.go` around lines 262 - 264, The type assertion
http.DefaultTransport.(*http.Transport) in newHTTPClient can panic if
DefaultTransport is not an *http.Transport; change it to a safe type-assertion
with the comma-ok idiom, e.g., attempt to cast into *http.Transport and if that
fails create a sensible default transport (or return an error) before calling
Clone(); ensure the variable transport is always a valid *http.Transport (either
the cloned one or a newly constructed &http.Transport{}) so downstream proxy
logic can safely use it.
Comment on lines
+34
to
+41
| func (s *FileWorkflowStore) Get(name string) (kernel.WorkflowResource, error) { | ||
| if err := validateWorkflowName(name); err != nil { | ||
| return kernel.WorkflowResource{}, err | ||
| } | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| return s.read(name) | ||
| } |
Contributor
There was a problem hiding this comment.
Same deep-copy concern as MemoryWorkflowStore.
Get delegates to read() which returns a workflow directly after unmarshaling (line 118). If kernel.WorkflowResource contains reference-type fields, callers can mutate the returned object without affecting the file, but the inconsistency with the secrets store pattern (which uses deepCopy()) suggests workflows should also be protected. Apply the same fix as recommended for MemoryWorkflowStore.Get.
🤖 Prompt for 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.
In `@internal/store/file_workflow_store.go` around lines 34 - 41, Get in
FileWorkflowStore returns the unmarshaled kernel.WorkflowResource from read()
directly, allowing callers to mutate internal data; modify FileWorkflowStore.Get
to return a defensive deep copy (reuse the same deepCopy() used by
MemoryWorkflowStore.Get or create one matching secrets store behavior) so
callers receive a copy, not the original. Locate FileWorkflowStore.Get and the
read() method, call deepCopy() on the returned kernel.WorkflowResource before
returning (or implement an equivalent deep copy helper), and ensure locking
semantics remain unchanged.
Comment on lines
+29
to
+37
| func (s *MemoryWorkflowStore) Get(name string) (kernel.WorkflowResource, error) { | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| workflow, exists := s.workflows[name] | ||
| if !exists { | ||
| return kernel.WorkflowResource{}, ErrWorkflowNotFound | ||
| } | ||
| return workflow, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if WorkflowResource or nested types contain maps/slices that require deep copy protection.
# Find WorkflowResource, Metadata, and WorkflowSpec definitions
ast-grep --pattern $'type WorkflowResource struct {
$$$
}'
ast-grep --pattern $'type Metadata struct {
$$$
}'
ast-grep --pattern $'type WorkflowSpec struct {
$$$
}'Repository: rezaqomy/FlowForge
Length of output: 1560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect MemoryWorkflowStore Get/List
FILE="internal/store/memory_workflow_store.go"
echo "=== $FILE (context) ==="
nl -ba "$FILE" | sed -n '1,140p'
# 2) Find any existing deepCopy helpers for workflow resource/spec
echo "=== rg deepCopy in internal/kernel/internal/store ==="
rg -n "deepCopy\\(" internal/kernel internal/store internal/secrets || true
echo "=== rg deepCopy word anywhere ==="
rg -n "deepCopy" internal || true
# 3) Inspect kernel workflow related nested types (TriggerDef, StepDef, etc.)
echo "=== Workflow nested types ==="
nl -ba internal/kernel/workflow.go | sed -n '1,220p'
# 4) Show how secrets store Get deep-copies (to compare pattern)
echo "=== internal/secrets/file_store.go (context around Get) ==="
nl -ba internal/secrets/file_store.go | sed -n '1,140p'
# 5) Check internal/store/memory_workflow_store.go for List implementation (if present)
echo "=== rg MemoryWorkflowStore methods ==="
rg -n "func \\(s \\*MemoryWorkflowStore\\)" internal/store/memory_workflow_store.goRepository: rezaqomy/FlowForge
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/store/memory_workflow_store.go"
echo "=== $FILE (context) ==="
cat -n "$FILE" | sed -n '1,220p'
echo "=== rg deepCopy in internal/kernel/internal/store internal/secrets ==="
rg -n "deepCopy\\(" internal/kernel internal/store internal/secrets || true
echo "=== rg deepCopy word anywhere ==="
rg -n "deepCopy" internal || true
echo "=== internal/kernel/workflow.go (focus nested types) ==="
cat -n internal/kernel/workflow.go | sed -n '1,260p'
echo "=== internal/secrets/file_store.go (context around Get + deepCopy) ==="
cat -n internal/secrets/file_store.go | sed -n '1,200p'Repository: rezaqomy/FlowForge
Length of output: 11361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for workflow deepCopy/copy helpers ==="
rg -n "WorkflowResource|WorkflowSpec|TriggerDef|StepDef" internal/kernel internal/store internal/secrets | head -n 200
rg -n "deepCopy\\(" internal/kernel internal/store internal/secrets || true
rg -n "copy\\(" internal/kernel internal/store || true
rg -n "List\\(\\) \\(\\[\\]kernel\\.WorkflowResource" -S internal || true
echo "=== Find other workflow store implementations (possible deep copy) ==="
fd -t f -e go internal/store | rg -n "workflow" || true
fd -t f internal/store | rg -n "workflow.*store" || true
# Print any other store files that match
fd -e go internal/store -a | rg -n "workflow" || trueRepository: rezaqomy/FlowForge
Length of output: 6914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/store/file_workflow_store.go ==="
cat -n internal/store/file_workflow_store.go | sed -n '1,240p'
echo "=== internal/store/memory_workflow_store_test.go ==="
cat -n internal/store/memory_workflow_store_test.go | sed -n '1,220p'
echo "=== internal/store/workflow_store.go ==="
cat -n internal/store/workflow_store.go | sed -n '1,120p'
echo "=== usages: .Get( ... ) on workflow store ==="
rg -n "\\.Get\\(.*\\)" internal | rg -n "Workflow|workflow" | head -n 80Repository: rezaqomy/FlowForge
Length of output: 9142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== internal/api/workflows.go ==="
cat -n internal/api/workflows.go | sed -n '1,200p'
echo "=== search: workflow/spec mutation ==="
rg -n "Spec\\.|Metadata\\.|Labels\\b|Annotations\\b|Trigger\\b|Steps\\b" internal/kernel internal/api internal/store | rg -n "\\=" || trueRepository: rezaqomy/FlowForge
Length of output: 4126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== workflow resource mutation candidates ==="
rg -n --glob='*.go' "Spec\\.\\w+\\s*=|Metadata\\.\\w+\\s*=|Labels\\[|Annotations\\[|With\\[|Then\\s*=|Else\\s*=" internal | head -n 200
echo "=== direct 'workflow.' mutations ==="
rg -n --glob='*.go' "workflow\\.(Spec|Metadata|Metadata\\.Labels|Metadata\\.Annotations|Trigger|Steps)\\b" internal | head -n 200
echo "=== check engine.runStep if it mutates StepDef/WorkflowSpec/TriggerDef ==="
rg -n --glob='*.go' "runStep|runSteps" internal/kernel/engine.go
cat -n internal/kernel/engine.go | sed -n '1,220p'Repository: rezaqomy/FlowForge
Length of output: 11223
Prevent mutable workflow reference leakage in MemoryWorkflowStore
kernel.WorkflowResource contains reference-type fields (Metadata.Labels/Annotations as map[string]string, TriggerDef.With + StepDef.With as map[string]any, and StepDef.Then/Else as []StepDef). internal/store/memory_workflow_store.go stores kernel.WorkflowResource directly in a map and Get/List return it without any deep copy, so callers that mutate the returned maps/slices can mutate the store’s internal workflow instances. (By contrast, FileWorkflowStore re-hydrates from JSON on each read, so it doesn’t share backing references with in-memory store state.)
Add a deepCopy() for kernel.WorkflowResource (recursively cloning nested maps/slices) and use it in MemoryWorkflowStore at least for Get and List (and ideally also when accepting workflows in Create/Save/Update to prevent aliasing on write).
🤖 Prompt for 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.
In `@internal/store/memory_workflow_store.go` around lines 29 - 37,
MemoryWorkflowStore currently returns and stores kernel.WorkflowResource by
reference, allowing callers to mutate internal maps/slices; add a deepCopy
function for kernel.WorkflowResource that recursively clones Metadata.Labels,
Metadata.Annotations, TriggerDef.With, each StepDef.With, StepDef.Then/Else
slices and any nested maps/slices, then use this deepCopy in
MemoryWorkflowStore.Get and MemoryWorkflowStore.List to return copies and also
use it on write paths (MemoryWorkflowStore.Create, Save, Update) to store a
cloned instance so the internal map never shares backing references with
callers.
Comment on lines
+66
to
+78
| func (s *MemoryWorkflowStore) List() ([]kernel.WorkflowResource, error) { | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
|
|
||
| out := make([]kernel.WorkflowResource, 0, len(s.workflows)) | ||
| for _, workflow := range s.workflows { | ||
| out = append(out, workflow) | ||
| } | ||
| sort.Slice(out, func(i, j int) bool { | ||
| return out[i].Metadata.Name < out[j].Metadata.Name | ||
| }) | ||
| return out, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Same deep-copy concern applies to List.
Like Get, List appends workflow structs by value (line 72), which may expose internal mutable state if WorkflowResource contains reference types. Apply the same deep-copy fix as recommended for Get.
🤖 Prompt for 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.
In `@internal/store/memory_workflow_store.go` around lines 66 - 78,
MemoryWorkflowStore.List currently appends the stored WorkflowResource values
directly from s.workflows into out, which can leak internal mutable state if
WorkflowResource contains reference types; change the loop in List to append
deep copies instead of original references by creating a copy for each workflow
(e.g., invoke an existing Clone/DeepCopy method on kernel.WorkflowResource or
implement one) and append that copy to out; keep the same sort on
out[i].Metadata.Name and return the copied slice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation