feat(tui): add cloud config form, ping command, and save flow#608
Open
mgaldamez wants to merge 22 commits into
Open
feat(tui): add cloud config form, ping command, and save flow#608mgaldamez wants to merge 22 commits into
mgaldamez wants to merge 22 commits into
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:
📝 WalkthroughWalkthroughAdds cloud sync settings to the TUI, including persisted server configuration, token fallback handling, URL validation, asynchronous health checks, save behavior, navigation, rendering, and tests. Store data-directory access and troubleshooting documentation are also updated. ChangesCloud sync settings
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds the TUI-side “Cloud sync settings” flow for configuring a cloud server URL, validating it by pinging the server health endpoint, and persisting the URL to cloud.json (while preserving any existing token fallback).
Changes:
- Adds new TUI screens for Cloud Settings and a Cloud Config form, including token source display and ping-before-save flow.
- Introduces
internal/tui/cloud.gofor loading/savingcloud.json, resolving token source, validating URLs, and pinging/healthwith an injectable transport. - Exposes
Store.DataDir()for TUI access and adds tests + docs updates aroundcloud.json.tokenfallback behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/view.go | Adds Cloud Settings/Config views and shared menu rendering |
| internal/tui/view_test.go | Adds view coverage for cloud config token source rendering |
| internal/tui/update.go | Adds navigation + key handling + ping/save state machine for cloud config |
| internal/tui/update_test.go | Adds navigation and save-flow tests (reachable/unreachable/401/preserve token) |
| internal/tui/model.go | Adds new screens, cloud config state, and loadCloudConfig command |
| internal/tui/model_test.go | Adds constants/command coverage for new cloud screens/config load |
| internal/tui/cloud.go | Implements config IO, token source resolution, URL validation, and ping logic |
| internal/tui/cloud_test.go | Adds unit tests for config IO, token source, ping, and URL validation |
| internal/store/store.go | Adds DataDir() accessor used by the TUI |
| internal/store/store_test.go | Adds a basic test for DataDir() |
| docs/engram-cloud/troubleshooting.md | Documents the intentional cloud.json.token fallback with env precedence |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+222
to
231
| if m.CloudConfigSaving { | ||
| b.WriteString("\n") | ||
| b.WriteString(m.SetupSpinner.View()) | ||
| b.WriteString(" Pinging server...") | ||
| b.WriteString("\n") | ||
| } else if m.CloudConfigPingStatus != "" { | ||
| b.WriteString("\n") | ||
| b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus)) | ||
| b.WriteString("\n") | ||
| } |
Comment on lines
+107
to
+110
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) | ||
| if err != nil { | ||
| return "unreachable", err | ||
| } |
Comment on lines
+668
to
+686
| case "enter", " ": | ||
| switch m.Cursor { | ||
| case 0: // Configure server | ||
| m.PrevScreen = ScreenCloudSettings | ||
| m.Screen = ScreenCloudConfig | ||
| m.Cursor = 0 | ||
| m.CloudConfigFocus = cloudConfigFocusInput | ||
| m.CloudConfigError = "" | ||
| m.CloudConfigPingStatus = "" | ||
| m.CloudConfigSaving = false | ||
| m.CloudConfigTest = false | ||
| m.CloudConfigInput.SetValue("") | ||
| m.CloudConfigInput.Focus() | ||
| return m, loadCloudConfigCmd(m.store.DataDir()) | ||
| case len(cloudSettingsMenuItems) - 1: // Back | ||
| m.Screen = ScreenDashboard | ||
| m.Cursor = 0 | ||
| return m, loadStats(m.store) | ||
| } |
Comment on lines
+61
to
+66
| cc.ServerURL = serverURL | ||
| b, err := json.MarshalIndent(cc, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o644) |
Comment on lines
+140
to
+164
| if msg.err != nil { | ||
| m.CloudConfigError = msg.err.Error() | ||
| return m, nil | ||
| } | ||
| if m.CloudConfigTest { | ||
| // Test-only ping: just show the status. | ||
| return m, nil | ||
| } | ||
| if msg.status == "reachable" || msg.status == "unauthorized" { | ||
| validatedURL, err := validateCloudServerURL(m.CloudConfigInput.Value()) | ||
| if err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| if err := saveCloudConfig(m.store.DataDir(), validatedURL); err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| m.Screen = ScreenCloudSettings | ||
| m.Cursor = 0 | ||
| m.CloudConfigFocus = cloudConfigFocusInput | ||
| return m, nil | ||
| } | ||
| m.CloudConfigError = "server is unreachable" | ||
| return m, nil |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/tui/cloud.go`:
- Around line 53-67: Update saveCloudConfig to write cloud.json with owner-only
permissions by changing the os.WriteFile file mode from 0o644 to 0o600, while
preserving the existing configuration and token handling.
- Around line 101-154: Normalize the validated URL before constructing the
request in pingCloudServerStatus so appending cloudHealthPath cannot produce a
double slash when the server URL has a trailing slash. Use url.JoinPath or
equivalent trailing-slash trimming while preserving the existing URL validation
behavior.
In `@internal/tui/update.go`:
- Around line 126-135: Update the cloudConfigLoadedMsg handler to avoid
unconditionally focusing CloudConfigInput after loading. Preserve the user's
current navigation/focus state when the message arrives, or update
CloudConfigFocus consistently whenever the widget is focused so key routing
remains synchronized with CloudConfigInput.Focused().
- Around line 137-164: Update the cloud ping flow around cloudPingMsg so the
async result carries the exact URL that was pinged, and use that URL in the
saveCloudConfig branch instead of re-reading m.CloudConfigInput.Value(). Ensure
the ping request snapshots or validates the URL before dispatch, and prevent
cloud-config input edits or focus changes while CloudConfigSaving is true.
In `@internal/tui/view.go`:
- Around line 185-236: Update viewCloudConfig to render m.CloudConfigError when
it is non-empty, including validation failures that occur before any ping status
is set. Add the error display near the existing CloudConfigPingStatus feedback
while preserving the current saving and status rendering behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 23defc48-1322-4bfb-8f6d-f80b7adce193
📒 Files selected for processing (11)
docs/engram-cloud/troubleshooting.mdinternal/store/store.gointernal/store/store_test.gointernal/tui/cloud.gointernal/tui/cloud_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/update.gointernal/tui/update_test.gointernal/tui/view.gointernal/tui/view_test.go
Comment on lines
+53
to
+67
| func saveCloudConfig(dataDir, serverURL string) error { | ||
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | ||
| return err | ||
| } | ||
| cc, err := loadCloudConfig(dataDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| cc.ServerURL = serverURL | ||
| b, err := json.MarshalIndent(cc, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o644) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
cloud.json written with world/group-readable permissions.
saveCloudConfig preserves the existing Token field and writes the file with 0o644, which any local user can read. This file can hold a bearer credential (tuiCloudConfig.Token), so it should be written with owner-only permissions.
🔒 Proposed fix
- return os.WriteFile(cloudConfigPath(dataDir), b, 0o644)
+ return os.WriteFile(cloudConfigPath(dataDir), b, 0o600)📝 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 saveCloudConfig(dataDir, serverURL string) error { | |
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | |
| return err | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err != nil { | |
| return err | |
| } | |
| cc.ServerURL = serverURL | |
| b, err := json.MarshalIndent(cc, "", " ") | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o644) | |
| } | |
| func saveCloudConfig(dataDir, serverURL string) error { | |
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | |
| return err | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err != nil { | |
| return err | |
| } | |
| cc.ServerURL = serverURL | |
| b, err := json.MarshalIndent(cc, "", " ") | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o600) | |
| } |
🤖 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/tui/cloud.go` around lines 53 - 67, Update saveCloudConfig to write
cloud.json with owner-only permissions by changing the os.WriteFile file mode
from 0o644 to 0o600, while preserving the existing configuration and token
handling.
Comment on lines
+137
to
+164
| case cloudPingMsg: | ||
| m.CloudConfigSaving = false | ||
| m.CloudConfigPingStatus = msg.status | ||
| if msg.err != nil { | ||
| m.CloudConfigError = msg.err.Error() | ||
| return m, nil | ||
| } | ||
| if m.CloudConfigTest { | ||
| // Test-only ping: just show the status. | ||
| return m, nil | ||
| } | ||
| if msg.status == "reachable" || msg.status == "unauthorized" { | ||
| validatedURL, err := validateCloudServerURL(m.CloudConfigInput.Value()) | ||
| if err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| if err := saveCloudConfig(m.store.DataDir(), validatedURL); err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| m.Screen = ScreenCloudSettings | ||
| m.Cursor = 0 | ||
| m.CloudConfigFocus = cloudConfigFocusInput | ||
| return m, nil | ||
| } | ||
| m.CloudConfigError = "server is unreachable" | ||
| return m, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'type cloudPingMsg' -A 6 internal/tui/cloud.go
rg -n 'func pingCloudServer' -A 20 internal/tui/cloud.goRepository: Gentleman-Programming/engram
Length of output: 1024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- internal/tui/cloud.go outline ---'
ast-grep outline internal/tui/cloud.go --view expanded || true
echo
echo '--- internal/tui/update.go outline ---'
ast-grep outline internal/tui/update.go --view expanded || true
echo
echo '--- cloudPingMsg definition / ping / save / key handlers ---'
rg -n 'type cloudPingMsg|func pingCloudServer|func saveCloudConfig|handleCloudConfigKeys|handleCloudConfigInputKeys|CloudConfigSaving|CloudConfigInput.Value\(\)' -A 40 -B 8 internal/tui/*.goRepository: Gentleman-Programming/engram
Length of output: 46396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '760,820p' internal/tui/update.go | cat -n
printf '\n---\n'
sed -n '1,120p' internal/tui/model.go | cat -n
printf '\n--- tests around cloud config focus/save ---\n'
sed -n '355,560p' internal/tui/update_test.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 13678
Persist the URL that was pinged
cloudPingMsg only carries status/error, so the save branch re-reads m.CloudConfigInput.Value() after the async ping finishes. Since focus changes and typing are still allowed while CloudConfigSaving is true, the user can save a different URL than the one that was actually tested. Pass the validated URL through the ping result (or snapshot it in state) and block cloud-config edits/focus changes until the save completes.
🤖 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/tui/update.go` around lines 137 - 164, Update the cloud ping flow
around cloudPingMsg so the async result carries the exact URL that was pinged,
and use that URL in the saveCloudConfig branch instead of re-reading
m.CloudConfigInput.Value(). Ensure the ping request snapshots or validates the
URL before dispatch, and prevent cloud-config input edits or focus changes while
CloudConfigSaving is true.
Source: Coding guidelines
Comment on lines
+185
to
+236
| func (m Model) viewCloudConfig() string { | ||
| var b strings.Builder | ||
|
|
||
| b.WriteString(headerStyle.Render(" Configure cloud server")) | ||
| b.WriteString("\n\n") | ||
|
|
||
| b.WriteString(detailLabelStyle.Render("Server URL:")) | ||
| b.WriteString(" ") | ||
| if m.CloudConfigFocus == cloudConfigFocusInput { | ||
| b.WriteString(searchInputStyle.Render(m.CloudConfigInput.View())) | ||
| } else { | ||
| b.WriteString(detailValueStyle.Render(m.CloudConfigInput.View())) | ||
| } | ||
| b.WriteString("\n\n") | ||
|
|
||
| b.WriteString(detailLabelStyle.Render("Token:")) | ||
| b.WriteString(" ") | ||
| b.WriteString(detailValueStyle.Render(m.CloudConfigTokenSource)) | ||
| b.WriteString("\n") | ||
| if m.CloudConfigTokenSource != TokenSourceEnv { | ||
| b.WriteString(timestampStyle.Render(" Set ENGRAM_CLOUD_TOKEN to override cloud.json.token")) | ||
| b.WriteString("\n") | ||
| } | ||
| b.WriteString("\n") | ||
|
|
||
| buttons := []string{"[Test]", "[Save]", "[Cancel]"} | ||
| for i, label := range buttons { | ||
| focus := i + 1 // input is 0 | ||
| if focus == m.CloudConfigFocus { | ||
| b.WriteString(menuSelectedStyle.Render("▸ " + label)) | ||
| } else { | ||
| b.WriteString(menuItemStyle.Render(" " + item)) | ||
| b.WriteString(menuItemStyle.Render(" " + label)) | ||
| } | ||
| b.WriteString(" ") | ||
| } | ||
| b.WriteString("\n") | ||
|
|
||
| if m.CloudConfigSaving { | ||
| b.WriteString("\n") | ||
| b.WriteString(m.SetupSpinner.View()) | ||
| b.WriteString(" Pinging server...") | ||
| b.WriteString("\n") | ||
| } else if m.CloudConfigPingStatus != "" { | ||
| b.WriteString("\n") | ||
| b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus)) | ||
| b.WriteString("\n") | ||
| } | ||
|
|
||
| // Help | ||
| b.WriteString(helpStyle.Render("\n j/k navigate • enter select • s search • q quit")) | ||
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) | ||
|
|
||
| return b.String() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CloudConfigError is set by the update flow but never rendered in this view.
m.CloudConfigError is populated for malformed-URL validation failures, ping transport errors, and post-ping re-validation failures, but viewCloudConfig() never writes it out. For the malformed-URL case specifically, no ping command is ever fired (see TestCloudConfigSaveInvalidURLNoPersist), so CloudConfigPingStatus also stays empty — the user sees no feedback at all after hitting Save on an invalid URL.
🐛 Proposed fix
b.WriteString(detailLabelStyle.Render("Token:"))
b.WriteString(" ")
b.WriteString(detailValueStyle.Render(m.CloudConfigTokenSource))
b.WriteString("\n")
+ if m.CloudConfigError != "" {
+ b.WriteString(errorStyle.Render(" " + m.CloudConfigError))
+ b.WriteString("\n")
+ }
if m.CloudConfigTokenSource != TokenSourceEnv {📝 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 (m Model) viewCloudConfig() string { | |
| var b strings.Builder | |
| b.WriteString(headerStyle.Render(" Configure cloud server")) | |
| b.WriteString("\n\n") | |
| b.WriteString(detailLabelStyle.Render("Server URL:")) | |
| b.WriteString(" ") | |
| if m.CloudConfigFocus == cloudConfigFocusInput { | |
| b.WriteString(searchInputStyle.Render(m.CloudConfigInput.View())) | |
| } else { | |
| b.WriteString(detailValueStyle.Render(m.CloudConfigInput.View())) | |
| } | |
| b.WriteString("\n\n") | |
| b.WriteString(detailLabelStyle.Render("Token:")) | |
| b.WriteString(" ") | |
| b.WriteString(detailValueStyle.Render(m.CloudConfigTokenSource)) | |
| b.WriteString("\n") | |
| if m.CloudConfigTokenSource != TokenSourceEnv { | |
| b.WriteString(timestampStyle.Render(" Set ENGRAM_CLOUD_TOKEN to override cloud.json.token")) | |
| b.WriteString("\n") | |
| } | |
| b.WriteString("\n") | |
| buttons := []string{"[Test]", "[Save]", "[Cancel]"} | |
| for i, label := range buttons { | |
| focus := i + 1 // input is 0 | |
| if focus == m.CloudConfigFocus { | |
| b.WriteString(menuSelectedStyle.Render("▸ " + label)) | |
| } else { | |
| b.WriteString(menuItemStyle.Render(" " + item)) | |
| b.WriteString(menuItemStyle.Render(" " + label)) | |
| } | |
| b.WriteString(" ") | |
| } | |
| b.WriteString("\n") | |
| if m.CloudConfigSaving { | |
| b.WriteString("\n") | |
| b.WriteString(m.SetupSpinner.View()) | |
| b.WriteString(" Pinging server...") | |
| b.WriteString("\n") | |
| } else if m.CloudConfigPingStatus != "" { | |
| b.WriteString("\n") | |
| b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus)) | |
| b.WriteString("\n") | |
| } | |
| // Help | |
| b.WriteString(helpStyle.Render("\n j/k navigate • enter select • s search • q quit")) | |
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) | |
| return b.String() | |
| } | |
| func (m Model) viewCloudConfig() string { | |
| var b strings.Builder | |
| b.WriteString(headerStyle.Render(" Configure cloud server")) | |
| b.WriteString("\n\n") | |
| b.WriteString(detailLabelStyle.Render("Server URL:")) | |
| b.WriteString(" ") | |
| if m.CloudConfigFocus == cloudConfigFocusInput { | |
| b.WriteString(searchInputStyle.Render(m.CloudConfigInput.View())) | |
| } else { | |
| b.WriteString(detailValueStyle.Render(m.CloudConfigInput.View())) | |
| } | |
| b.WriteString("\n\n") | |
| b.WriteString(detailLabelStyle.Render("Token:")) | |
| b.WriteString(" ") | |
| b.WriteString(detailValueStyle.Render(m.CloudConfigTokenSource)) | |
| b.WriteString("\n") | |
| if m.CloudConfigError != "" { | |
| b.WriteString(errorStyle.Render(" " + m.CloudConfigError)) | |
| b.WriteString("\n") | |
| } | |
| if m.CloudConfigTokenSource != TokenSourceEnv { | |
| b.WriteString(timestampStyle.Render(" Set ENGRAM_CLOUD_TOKEN to override cloud.json.token")) | |
| b.WriteString("\n") | |
| } | |
| b.WriteString("\n") | |
| buttons := []string{"[Test]", "[Save]", "[Cancel]"} | |
| for i, label := range buttons { | |
| focus := i + 1 // input is 0 | |
| if focus == m.CloudConfigFocus { | |
| b.WriteString(menuSelectedStyle.Render("▸ " + label)) | |
| } else { | |
| b.WriteString(menuItemStyle.Render(" " + label)) | |
| } | |
| b.WriteString(" ") | |
| } | |
| b.WriteString("\n") | |
| if m.CloudConfigSaving { | |
| b.WriteString("\n") | |
| b.WriteString(m.SetupSpinner.View()) | |
| b.WriteString(" Pinging server...") | |
| b.WriteString("\n") | |
| } else if m.CloudConfigPingStatus != "" { | |
| b.WriteString("\n") | |
| b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus)) | |
| b.WriteString("\n") | |
| } | |
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) | |
| return b.String() | |
| } |
🤖 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/tui/view.go` around lines 185 - 236, Update viewCloudConfig to
render m.CloudConfigError when it is non-empty, including validation failures
that occur before any ping status is set. Add the error display near the
existing CloudConfigPingStatus feedback while preserving the current saving and
status rendering behavior.
This was referenced Jul 14, 2026
Contributor
Author
|
@coderabbitai re-review please — just added godoc comments to new exported functions (commit 6dde3ce on this branch's base, and b129225 on the enrollment branch). Want to confirm the docstring coverage warning is now resolved. Thanks! |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd cloud.go internal/tui
echo "---cloud.go---"
cat -n internal/tui/cloud.go 2>/dev/null | head -80
echo "---config load functions---"
rg -n "^func " internal/tui/cloud.go
echo "---check comments above exported funcs/consts---"
rg -n -B3 "^func (Load|Save|New|Ping|TokenSource|ValidateCloudServerURL)" internal/tui/cloud.go
rg -n -B3 "^func " internal/store/store.goLength of output: 3885 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---store.go DataDir---"
rg -n -B3 "^func \(s \*Store\) DataDir" internal/store/store.go
echo "---model.go new funcs---"
rg -n -B3 "^func " internal/tui/model.go | sed -n '1,100p'
echo "---exported funcs across tui---"
rg -n "^func [A-Z]" internal/tui/*.go
echo "---exported funcs store---"
rg -n "^func [A-Z]|^func \(.*\) [A-Z]" internal/store/store.goLength of output: 16811 Confirmed — the new exported functions/constants introduced in this PR now have godoc comments in place:
This resolves the docstring coverage gap I flagged earlier. Thanks for the quick turnaround! ✅ Action performedReview finished.
|
Collaborator
|
Reviewed as part of the #607–#612 series — full feedback is on #607 to keep it in one place. Short version: |
Alan-TheGentleman
added
type:feature
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 `@internal/cloudconfig/cloudconfig.go`:
- Around line 75-82: Update Save to write the cloud-config data to a temporary
file in the same directory, close it successfully, normalize its mode, and
atomically rename it over the target path only after all prior operations
succeed. Preserve the existing 0o644 permissions and return errors without
replacing the current cloud-config file.
- Around line 57-66: Make directory permissions stable in Save and align its
test: after os.MkdirAll creates or verifies dataDir, explicitly apply mode 0o755
with os.Chmod and propagate any error; update the corresponding cloudconfig test
to verify the resulting directory mode while preserving the existing file-mode
assertions.
- Around line 57-82: Update Save in internal/cloudconfig/cloudconfig.go (lines
57-82) to write and chmod cloud.json with owner-only permissions 0o600, and
revise the summary/description to state this mode. Update the new-file
expectation in internal/cloudconfig/cloudconfig_test.go (lines 115-125) and the
overwrite-normalization expectation (lines 208-214) to expect 0o600.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 84ab649f-82e0-4fcd-a2c1-a5d577bfafee
📒 Files selected for processing (2)
internal/cloudconfig/cloudconfig.gointernal/cloudconfig/cloudconfig_test.go
Comment on lines
+57
to
+82
| // Save writes cfg to cloud.json inside dataDir. The data directory | ||
| // is created with mode 0o755 if it does not exist; the file is | ||
| // written with mode 0o644. | ||
| // | ||
| // os.WriteFile only applies the permission bits on file creation, so | ||
| // a pre-existing file would keep whatever mode it already had. Save | ||
| // chmods the file after writing to guarantee the on-disk mode is | ||
| // always 0o644, regardless of prior state. | ||
| func Save(dataDir string, cfg *Config) error { | ||
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| data, err := json.Marshal(cfg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| path := Path(dataDir) | ||
| if err := os.WriteFile(path, data, 0o644); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Normalize the file mode on every write so existing files do not | ||
| // retain a stale, non-spec permission. | ||
| return os.Chmod(path, 0o644) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C2 'Token|0o644|0o600|cloud\.json' internal/cloudconfigRepository: Gentleman-Programming/engram
Length of output: 12521
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files mentioning cloudconfig load/save/usages =="
rg -n -C2 'cloudconfig|CloudConfig|config.Cloud|internal/cloudconfig|Load\(.*Config|Token|cfg\.Token|token' -g '!internal/cloudconfig/*'
echo
echo "== source outline =="
ast-grep outline internal/cloudconfig/cloudconfig.go || trueRepository: Gentleman-Programming/engram
Length of output: 318
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== repository references to cloudconfig package =="
git ls-files | while IFS= read -r f; do
case "$f" in
internal/cloudconfig/*) ;;
*) if grep -qE 'cloudconfig|cloud\.json|Token|ServerURL|Save\(.*Config|Load\(' "$f" 2>/dev/null; then
echo "--- $f ---"
grep -n -C2 -E 'cloudconfig|cloud\.json|Token|ServerURL|Save\(.*Config|Load\(' "$f" || true
fi ;;
esac
done
echo
echo "== cloudconfig source/test key sections =="
sed -n '1,240p' internal/cloudconfig/cloudconfig.go
printf '\n--- test ---\n'
sed -n '80,226p' internal/cloudconfig/cloudconfig_test.goRepository: Gentleman-Programming/engram
Length of output: 50385
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== direct imports/usages of internal/cloudconfig =="
rg -n 'cloudconfig\.(|Path\(|Load\(|Save\(|Config)' . --glob \*.{go,md} --glob '!internal/cloudconfig/*' --glob '!internal/tui/*' || true
echo
echo "== cloudconfig source =="
cat -n internal/cloudconfig/cloudconfig.go
echo
echo "== cloudconfig tests =="
cat -n internal/cloudconfig/cloudconfig_test.goRepository: Gentleman-Programming/engram
Length of output: 11934
Protect persisted tokens with owner-only file permissions. Save serializes Config.Token but writes and normalizes cloud.json as 0o644, making it readable by other local users.
internal/cloudconfig/cloudconfig.go#L57-L82: write and chmodcloud.jsonto0o600; update the summary/description accordingly.internal/cloudconfig/cloudconfig_test.go#L115-L125: expect0o600for newly written config files.internal/cloudconfig/cloudconfig_test.go#L208-L214: expect0o600after overwrite normalization.
📍 Affects 2 files
internal/cloudconfig/cloudconfig.go#L57-L82(this comment)internal/cloudconfig/cloudconfig_test.go#L115-L125internal/cloudconfig/cloudconfig_test.go#L208-L214
🤖 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/cloudconfig/cloudconfig.go` around lines 57 - 82, Update Save in
internal/cloudconfig/cloudconfig.go (lines 57-82) to write and chmod cloud.json
with owner-only permissions 0o600, and revise the summary/description to state
this mode. Update the new-file expectation in
internal/cloudconfig/cloudconfig_test.go (lines 115-125) and the
overwrite-normalization expectation (lines 208-214) to expect 0o600.
Comment on lines
+75
to
+82
| path := Path(dataDir) | ||
| if err := os.WriteFile(path, data, 0o644); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Normalize the file mode on every write so existing files do not | ||
| // retain a stale, non-spec permission. | ||
| return os.Chmod(path, 0o644) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C2 'func Save|os\.(WriteFile|CreateTemp|Rename|Chmod)' internal/cloudconfig/cloudconfig.go
rg -n -C2 'TestSave|Save\(' internal/cloudconfig/cloudconfig_test.goRepository: Gentleman-Programming/engram
Length of output: 2495
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- cloudconfig.go outline ---'
ast-grep outline internal/cloudconfig/cloudconfig.go --view expanded || true
echo '--- cloudconfig.go relevant lines ---'
sed -n '1,120p' internal/cloudconfig/cloudconfig.go | cat -n
echo '--- cloudconfig_test.go relevant lines ---'
sed -n '80,230p' internal/cloudconfig/cloudconfig_test.go | cat -n
echo '--- config type and save/load callers summary ---'
rg -n 'type Config|Token:|ServerURL:|cloudconfig\.Save|cloudconfig\.Load|Secret|Token|ServerURL' internal go.mod 2>/dev/null || true
echo '--- deterministic os.WriteFile behavior probe via fs package source comments is not needed, read-only summary requested ---'Repository: Gentleman-Programming/engram
Length of output: 50385
Make cloud-config replacement atomic.
Save writes directly to cloud.json, so os.WriteFile truncates any existing file before JSON is flushed. If the write is interrupted, the persisted token/config can be lost; write to a same-directory temp file and rename it into place after a successful close.
🤖 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/cloudconfig/cloudconfig.go` around lines 75 - 82, Update Save to
write the cloud-config data to a temporary file in the same directory, close it
successfully, normalize its mode, and atomically rename it over the target path
only after all prior operations succeed. Preserve the existing 0o644 permissions
and return errors without replacing the current cloud-config file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/cloudconfig/token.go`:
- Around line 73-81: The environment token should remain opaque after non-empty
validation. In internal/cloudconfig/token.go:73-81, update EffectiveToken to
return the original environment value v while using strings.TrimSpace(v) only to
detect whether it is blank, and revise the surrounding comment to describe this
behavior. In internal/cloudconfig/token_test.go:86-91, update the expectation to
preserve the surrounding whitespace in the token value.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9776d3ef-fea3-4ab7-8d7e-923ddb0f9485
📒 Files selected for processing (2)
internal/cloudconfig/token.gointernal/cloudconfig/token_test.go
Comment on lines
+73
to
+81
| // Whitespace-only env values are treated as unset. Surrounding | ||
| // whitespace on a real env value is stripped before the value is | ||
| // returned, so a token in the form " e1 " surfaces as "e1". | ||
| // This matches the existing CLI's resolveCloudRuntimeConfig | ||
| // behavior at cmd/engram/main.go:447, where the trimmed value | ||
| // is what gets written to the runtime config. | ||
| func EffectiveToken(dataDir string) (string, Source) { | ||
| if v := os.Getenv(EnvCloudToken); strings.TrimSpace(v) != "" { | ||
| return strings.TrimSpace(v), SourceEnv |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the environment token value after using trimming only for emptiness detection.
The stated contract preserves non-whitespace ENGRAM_CLOUD_TOKEN values, but this returns "e1" for " e1 " and the test locks that behavior in.
internal/cloudconfig/token.go#L73-L81: return the originalvwhenstrings.TrimSpace(v) != ""; update the doc comment accordingly.internal/cloudconfig/token_test.go#L86-L91: expect" e1 "so the test enforces the intended opaque-token behavior.
Proposed fix
- return strings.TrimSpace(v), SourceEnv
+ return v, SourceEnv- wantToken: "e1",
+ wantToken: " e1 ",📝 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
| // Whitespace-only env values are treated as unset. Surrounding | |
| // whitespace on a real env value is stripped before the value is | |
| // returned, so a token in the form " e1 " surfaces as "e1". | |
| // This matches the existing CLI's resolveCloudRuntimeConfig | |
| // behavior at cmd/engram/main.go:447, where the trimmed value | |
| // is what gets written to the runtime config. | |
| func EffectiveToken(dataDir string) (string, Source) { | |
| if v := os.Getenv(EnvCloudToken); strings.TrimSpace(v) != "" { | |
| return strings.TrimSpace(v), SourceEnv | |
| // Whitespace-only env values are treated as unset. Surrounding | |
| // whitespace on a real env value is stripped before the value is | |
| // returned, so a token in the form " e1 " surfaces as "e1". | |
| // This matches the existing CLI's resolveCloudRuntimeConfig | |
| // behavior at cmd/engram/main.go:447, where the trimmed value | |
| // is what gets written to the runtime config. | |
| func EffectiveToken(dataDir string) (string, Source) { | |
| if v := os.Getenv(EnvCloudToken); strings.TrimSpace(v) != "" { | |
| return v, SourceEnv |
📍 Affects 2 files
internal/cloudconfig/token.go#L73-L81(this comment)internal/cloudconfig/token_test.go#L86-L91
🤖 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/cloudconfig/token.go` around lines 73 - 81, The environment token
should remain opaque after non-empty validation. In
internal/cloudconfig/token.go:73-81, update EffectiveToken to return the
original environment value v while using strings.TrimSpace(v) only to detect
whether it is blank, and revise the surrounding comment to describe this
behavior. In internal/cloudconfig/token_test.go:86-91, update the expectation to
preserve the surrounding whitespace in the token value.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/cloudconfig/server.go`:
- Around line 68-69: Clear ForceQuery alongside RawQuery in the URL cleanup
logic in internal/cloudconfig/server.go:68-69, preserving Fragment cleanup. Add
test cases in internal/cloudconfig/server_test.go:41-45 for https://example.com?
and https://example.com/path?, asserting both serialize without a trailing
question mark.
- Around line 65-66: The server URL validation in the relevant server parsing
function must use u.Hostname() == "" instead of checking u.Host, rejecting
port-only authorities such as https://:8080 and http://:8080. Add both URLs to
the existing missing-host test cases in server_test.go.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7463e444-7076-432c-ac76-0c6924fd345b
📒 Files selected for processing (2)
internal/cloudconfig/server.gointernal/cloudconfig/server_test.go
Comment on lines
+68
to
+69
| u.RawQuery = "" | ||
| u.Fragment = "" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- git files ---\n'
git ls-files | grep -E '(^|/)internal/cloudconfig/(server\.go|server_test\.go)$' || true
printf '\n--- relevant lines ---\n'
if [ -f internal/cloudconfig/server.go ]; then
sed -n '1,120p' internal/cloudconfig/server.go | cat -n
fi
if [ -f internal/cloudconfig/server_test.go ]; then
sed -n '1,90p' internal/cloudconfig/server_test.go | cat -n
fi
printf '\n--- ForceQuery references ---\n'
rg -n "ForceQuery|RawQuery|URL\(\)|String\(\)" internal/cloudconfig || true
printf '\n--- local url behavior probe ---\n'
python3 - <<'PY'
from urllib.parse import urlparse, urlunparse
# Use Python's net.url is not available directly for Go ForceQuery semantics; instead inspect installed Go docs?
PY
if command -v go >/dev/null 2>&1; then
cat >/tmp/forcequery_probe.go <<'GO'
package main
import (
"fmt"
"net/url"
)
func main() {
input, _ := url.Parse("https://example.com?")
before := input.String()
input.RawQuery = ""
after := input.String()
fmt.Printf("before=%q after=%q forceQuery=%v\n", before, after, input.ForceQuery)
}
GO
go run /tmp/forcequery_probe.go
fiRepository: Gentleman-Programming/engram
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- git files ---'
git ls-files | grep -E '(^|/)internal/cloudconfig/(server\.go|server_test\.go)$' || true
printf '%s\n' ''
printf '%s\n' '--- relevant lines ---'
if [ -f internal/cloudconfig/server.go ]; then
sed -n '1,120p' internal/cloudconfig/server.go | cat -n
fi
if [ -f internal/cloudconfig/server_test.go ]; then
sed -n '1,90p' internal/cloudconfig/server_test.go | cat -n
fi
printf '%s\n' ''
printf '%s\n' '--- ForceQuery references ---'
rg -n "ForceQuery|RawQuery|URL\(\)|String\(\)" internal/cloudconfig || true
printf '%s\n' ''
printf '%s\n' '--- local url behavior probe ---'
if command -v go >/dev/null 2>&1; then
cat >/tmp/forcequery_probe.go <<'GO'
package main
import (
"fmt"
"net/url"
)
func main() {
input, _ := url.Parse("https://example.com?")
before := input.String()
input.RawQuery = ""
after := input.String()
fmt.Printf("before=%q after=%q forceQuery=%v\n", before, after, input.ForceQuery)
}
GO
go run /tmp/forcequery_probe.go
else
printf '%s\n' 'go not available'
fiRepository: Gentleman-Programming/engram
Length of output: 7989
Clear ForceQuery when stripping query components.
https://example.com? parses with an empty query but still serializes as https://example.com?; setting only RawQuery = "" leaves ForceQuery set.
internal/cloudconfig/server.go#L68-L69: addu.ForceQuery = falsewith the query cleanup.internal/cloudconfig/server_test.go#L41-L45: addhttps://example.com?andhttps://example.com/path?cases expecting no trailing?.
📍 Affects 2 files
internal/cloudconfig/server.go#L68-L69(this comment)internal/cloudconfig/server_test.go#L41-L45
🤖 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/cloudconfig/server.go` around lines 68 - 69, Clear ForceQuery
alongside RawQuery in the URL cleanup logic in
internal/cloudconfig/server.go:68-69, preserving Fragment cleanup. Add test
cases in internal/cloudconfig/server_test.go:41-45 for https://example.com? and
https://example.com/path?, asserting both serialize without a trailing question
mark.
Source: Path instructions
RED-GREEN-TRIANGULATE-REFACTOR cycle for T-608.1: - Config struct (ServerURL, Token) - Path(dataDir) joins with cloud.json - Load returns zero-value on IsNotExist, error on malformed JSON - Save creates dir 0o755, file 0o644; chmod normalizes pre-existing files Tests cover Path, Load (not-exist, malformed, empty, round-trip), Save (creates dir/file, mode normalization on overwrite). Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
T-608.2. RED-GREEN-TRIANGULATE-REFACTOR cycle. Source enum (SourceNone, SourceFile, SourceEnv) with String() and SourceLabel(). EffectiveToken(dataDir) returns (token, source) with precedence: env var ENGRAM_CLOUD_TOKEN (when set and non-whitespace) overrides file token from cloud.json. Tests cover file+env, file-only, env-only, neither, whitespace env (treats as unset), empty file, and env-with-surrounding-spaces (value preserved). Triangulation adds nil/malformed Load fallback regression test. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
T-608.3. RED-GREEN-TRIANGULATE-REFACTOR cycle. ValidateServerURL(raw) returns the normalized URL with query and fragment cleared on success, or an error for empty/whitespace input, malformed URLs, non-http(s) schemes, or missing host. Trailing slash and path are preserved as-given. Tests cover http/https, bad schemes (ftp, file), missing scheme or host, query/fragment handling (cleared on success), path and userinfo preservation, IPv6 hosts. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
T-608.4. RED-GREEN-TRIANGULATE-REFACTOR cycle. LocalDaemonProbe(ctx, port) Result classifies the local daemon into ProbeRunning (2xx), ProbeNotRunning (*net.OpError dial), or ProbeUnreachable (other errors). ProbeTimeout (var, default 1s) and ProbeTransport (var, default http.DefaultTransport) provide the test seam per ADR-1, matching the existing cmd/engram/cloud_daemon_probe.go pattern. ResolvePort() honors ENGRAM_PORT env var, falling back to 7437 for unset, invalid, or out-of-range values. Tests cover 200, 500, timeout (via slowTransport + shortened ProbeTimeout), context cancellation, not-running (unused port, explicit *net.OpError assertion), and ResolvePort table (unset, valid low/high/typical/7437, non-numeric, zero, negative, out-of-range, empty, whitespace, trimmed-whitespace). Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
T-608.5. RED-GREEN-REFACTOR cycle. TestCLIAndTUIAgreeOnTokenSource pins the invariant that the CLI's SourceLabel and the TUI's view layer render the same string for the same (file, env) triple. Covers file-set/env-empty, file-empty/env-set, both-set/env-wins, and both-empty. REFACTOR: lift the label strings to exported consts (LabelSourceNone, LabelSourceFile, LabelSourceEnv) so the TUI and any other consumer can reference stable APIs and prevent drift. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
mgaldamez
force-pushed
the
feat/tui-cloud-config-02-config
branch
from
July 24, 2026 00:27
61183c2 to
d77e66d
Compare
added 4 commits
July 23, 2026 18:50
T-608.6. Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). Replaces the legacy validateCloudServerURL (which rejected URLs with ?q=1 or #frag) with cloudconfig.ValidateServerURL (which accepts and clears them) per spec REQ-1's deliberate spec change. This pins the TUI/CLI agreement: cloudconfig.ValidateServerURL is the single source of truth for URL validation. TestCloudUpgradeDoctorAcceptsQueryAndFragment covers the new behavior. TestCloudUpgradeDoctorAcceptsCleanURL guards the existing happy path. TestCloudUpgradeDoctorURLAcceptanceMatrix triangulates across query-only, fragment-only, both, path+query, port+query, and path+fragment inputs. The legacy validateCloudServerURL is kept for now; subsequent tasks (T-608.7, T-608.8) migrate the remaining call sites before the legacy function is deleted. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
…onfig.ValidateServerURL T-608.7. Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). Second validateCloudServerURL call site migration. Same behavior change as T-608.6: URLs with ?q=1 or #frag are now accepted and cleared (previously rejected), per spec REQ-1. The early-return guard 'if cc == nil || cc.ServerURL == ""' is preserved. TestCloudUpgradeBootstrapAcceptsQueryAndFragment covers the new behavior. TestCloudUpgradeBootstrapAcceptsCleanURL guards the existing happy path. TestCloudUpgradeBootstrapURLAcceptanceMatrix triangulates across query_only, fragment_only, both, path+query, port+query, and path+fragment inputs. TestCloudUpgradeBootstrapEarlyReturnOnEmptyServerURL pins the early-return guard so the migration does not change it. The legacy validateCloudServerURL function is kept; T-608.8 migrates the remaining call sites. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). Replaces validateCloudServerURL with cloudconfig.ValidateServerURL (URLs with ?q=1 / #frag now accepted and cleared, per spec REQ-1) and saveCloudConfig with cloudconfig.Save (preserves file mode 0o644 contract from T-608.1, plus the os.Chmod normalization for existing files). TestCloudConfigAcceptsQueryAndFragment pins the new behavior at the call site. TestCloudConfigURLAcceptanceMatrix triangulates across 6 subcases (query-only, fragment-only, both, path+query, port+query, path+fragment). TestCloudConfigLoadAfterSaveRoundTrip verifies JSON compatibility between the save and the new package's Load. TestCloudConfigCreatesMissingDirectory pins the MkdirAll contract. TestCloudConfigNormalizesFileMode pins the os.Chmod contract for files with pre-existing wrong modes. TestCmdCloudConfigRejectsInvalidServerURL (main_extra_test.go) is updated to remove the now-accepted URLs (queries and fragments), which are covered by TestCloudConfigURLAcceptanceMatrix. The legacy validateCloudServerURL and saveCloudConfig are preserved for T-608.15 deletion. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). Replaces os.ReadFile(cloudConfigPath(cfg)) with os.ReadFile(cloudconfig.Path(cfg.DataDir)) in the snapshot writeback path inside captureUpgradeSnapshotBeforeBootstrap (cmd/engram/cloud.go:550). CRITICAL: uses os.ReadFile directly (not cloudconfig.Load) because the snapshot needs raw JSON bytes, not decoded values. The snapshot is later used to ROLLBACK the upgrade, so losing any field the package's Config struct does not declare would be a silent data-loss bug. The path is read from cloudconfig.Path to align with the rest of the migration, but the read call stays os.ReadFile so the bytes are preserved unchanged. TestCloudUpgradeBootstrapSnapshotWritebackUsesRawBytes (in cloud_test.go) pins the contract: a cloud.json with a sentinel field that cloudconfig.Config does not declare must survive the snapshot. If the migration accidentally used cloudconfig.Load, the sentinel would be lost in decoding. TestCloudUpgradeBootstrapSnapshotWritebackPathMatchesCloudconfigPath asserts the snapshot's source file is at cloudconfig.Path. TestCloudUpgradeBootstrapSnapshotWritebackHandlesMissingFile pins the os.ErrNotExist graceful-handling branch. The legacy cloudConfigPath helper is preserved for T-608.10 (rollback writeback) and T-608.15 (deletion). Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Contributor
Author
|
still working |
added 7 commits
July 23, 2026 21:15
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). The 'if cc == nil || cc.ServerURL == ""' check in cmdCloudStatus (cmd/engram/cloud.go:676) is the contract the spec REQ-2 audit promises: a missing file, a zero-value config, or a file with empty ServerURL all reduce to 'Cloud status: not configured'. The 'cc == nil' half is defensive (resolveCloudRuntimeConfig currently converts nil to zero-value) but the rewrite keeps it so a future caller that returns nil cannot crash on a nil deref. Five new test functions pin the contract at the cmdCloudStatus level: - TestCloudStatusNoFileNoEnvReportsNotConfigured: no cloud.json, no env vars -> 'not configured' (zero-value case). - TestCloudStatusEmptyServerURLInConfigReportsNotConfigured: a cloud.json with empty server_url is treated as not configured regardless of any persisted token. - TestCloudStatusEmptyServerURLInConfigWithEnvServerReportsConfigured: ENGRAM_CLOUD_SERVER env override beats the file's empty URL. - TestCloudStatusNotConfiguredDoesNotInvokeDaemonProbe: the not-configured branch is short-circuit; no daemon probe, no sync diagnostic. - TestCloudStatusMalformedConfigSurfacesErrorNotNotConfigured: malformed cloud.json surfaces a parse error, not 'not configured' (negative-space guard). The current code at cloud.go:676 is already in the rewritten form; the tests are regression-pinning approval tests per the strict-tdd module's refactor-existing-code guidance. The legacy 'cc == nil' branch in resolveCloudRuntimeConfig is replaced by the new package's zero-value contract in T-608.12 (next slice). Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
…8.12) Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). resolveCloudRuntimeConfig now delegates to cloudconfig.Load (returns non-nil zero-value per T-608.1) then applies env overrides: ENGRAM_CLOUD_SERVER overrides ServerURL, ENGRAM_CLOUD_TOKEN overrides Token, both whitespace-trimmed. Empty/whitespace-only env values are treated as unset (file value preserved). The override logic is extracted to applyEnvOverrides for auditability. Return type changes from *cloudConfig to *cloudconfig.Config; all callers (cmdCloudUpgradeDoctor, cmdCloudUpgradeBootstrap, cmdCloudStatus, cloudSyncEnabled, preflightCloudSync, autosync in tryStartAutosync) use only cc.ServerURL / cc.Token field access, so no caller-side code change is required. The runUpgradeBootstrap var seam now takes *cloudconfig.Config; all test stubs that use it are updated. Eight new test functions pin the new contract: - TestResolveCloudRuntimeConfigReturnsCloudconfigConfigType: type-asserts the return is *cloudconfig.Config (catches regressions where the legacy type returns). - TestResolveCloudRuntimeConfigEmptyFileEmptyEnvReturnsZeroValue: no file, no env -> empty fields, non-nil. - TestResolveCloudRuntimeConfigAppliesServerEnvOverride: env SERVER wins over file. - TestResolveCloudRuntimeConfigAppliesTokenEnvOverride: env TOKEN wins over file; file ServerURL preserved. - TestResolveCloudRuntimeConfigAppliesBothEnvOverrides: both env vars apply independently. - TestResolveCloudRuntimeConfigEmptyEnvFallsBackToFile: no env -> file values. - TestResolveCloudRuntimeConfigTrimsWhitespaceEnv: leading/trailing whitespace is trimmed before apply. - TestResolveCloudRuntimeConfigWhitespaceOnlyEnvTreatedAsUnset: whitespace-only env -> file preserved. - TestResolveCloudRuntimeConfigMalformedFileDoesNotApplyEnv: parse error short-circuits the override (env not applied). The existing TestResolveCloudRuntimeConfig* tests (3 of them) continue to pass: the field access pattern (cc.ServerURL, cc.Token) is identical between the legacy *cloudConfig and the new *cloudconfig.Config. Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR). The autosync caller in tryStartAutosync (cmd/engram/main.go:808) accesses cc.Token and cc.ServerURL WITHOUT a nil guard, relying on the new cloudconfig.Load zero-value contract from T-608.1. The audit confirms the caller is correct: cc is never nil from a successful resolveCloudRuntimeConfig call. Four new test functions pin the contract at the tryStartAutosync level by stubbing newAutosyncManager and capturing the log output to verify the '[autosync] started (server=...)' line contains the expected URL: - TestAutosyncCallerReadsTokenFromEnv: env TOKEN/SERVER set -> the manager is created with the env values. - TestAutosyncCallerReadsTokenFromFile: env absent, file has token/server -> the manager is created with the file values (Windows Task Scheduler scenario, issue Gentleman-Programming#421). - TestAutosyncCallerEnvWinsOverFile: both env and file set -> env wins (the file URL must NOT appear in the log). - TestAutosyncCallerAbortsWhenNoConfig: no file, no env -> the manager factory is NOT called (return nil, nil). The tests use a small captureLog helper to redirect the package-level log output to a buffer; the buffer is read after tryStartAutosync returns. The log capture is restored via t.Cleanup. The existing TestAutosyncGating* tests (4 functions) and TestTryStartAutosyncReturnsStopFn continue to pass; the new tests are a complementary layer that asserts the value flowed through the caller (token, server URL) matches the expected source (file, env, both, empty). Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR).
Replaces defaultCloudDaemonProbe, daemonProbeStatus,
daemonProbeResult, daemonProbeTimeout (moved to
internal/cloudconfig in T-608.4) with calls to
cloudconfig.LocalDaemonProbe, cloudconfig.ProbeStatus,
cloudconfig.Result, cloudconfig.ProbeTimeout. Keeps
printCloudStatusDaemonProbe and resolveDaemonProbePort as
thin wrappers that delegate to the package's types. The
file shrinks from 102 lines to 72 (with documentation
comments — functional code is ~30 lines).
The local cloudDaemonProbe var is preserved as a thin
wrapper that defaults to cloudconfig.LocalDaemonProbe so
existing tests that stub the var continue to work without
importing the cloudconfig package's seam. T-608.15 will
rewrite the print-test to stub cloudconfig.LocalDaemonProbe
directly (the design ADR-1 seam).
All 4 TestDefaultCloudDaemonProbe* tests, the
TestResolveDaemonProbePortHonorsEnvAndDefaults test, and
the TestPrintCloudStatusDaemonProbeFormatsEachState test
are updated to use the new types. The behavior is
unchanged: the stdout output of cmdCloudStatus for the
'Local daemon:' line is byte-identical to the pre-migration
output. The cloudconfig.ProbeStatus.String() labels
('running', 'not running', 'unreachable') match the
pre-migration daemonProbeStatus constants.
TestCloudStatusNotConfiguredDoesNotInvokeDaemonProbe
(T-608.10) and other test files that stub cloudDaemonProbe
in main_extra_test.go are updated to use cloudconfig.Result
and cloudconfig.ProbeRunning / cloudconfig.ProbeNotRunning
constants.
Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR).
Ports TestPrintCloudStatusDaemonProbeFormatsEachState to stub
cloudconfig.LocalDaemonProbe via the new var seam. The local
cloudDaemonProbe wrapper in cmd/engram/cloud_daemon_probe.go
is now a function (not a var) that delegates to
cloudconfig.LocalDaemonProbe at call time, so test stubs of
the cloudconfig var reach printCloudStatusDaemonProbe
without a separate local seam. The var pattern in
cloudconfig.LocalDaemonProbe is the ADR-1 contract: it
defaults to localDaemonProbe and can be substituted by tests
or by future variants of the CLI.
Deletes the legacy cloudConfigPath, loadCloudConfig,
saveCloudConfig, validateCloudServerURL helpers from
cmd/engram/cloud.go. The cloudConfig struct is preserved
because cmdCloudConfig still uses it for the
(*cloudconfig.Config)(cc) type conversion when saving.
Imports 'encoding/json' and 'net/url' are removed from
cloud.go (no longer used after the deletions).
Migrates the remaining legacy call sites:
- cloud.go:644,649 cloudConfigPath -> cloudconfig.Path
- cloud.go:680 validateCloudServerURL -> cloudconfig.ValidateServerURL
- main.go:357,513 validateCloudServerURL -> cloudconfig.ValidateServerURL
- main_extra_test.go:1680 loadCloudConfig -> cloudconfig.Load
- 17 saveCloudConfig call sites in main_extra_test.go
- 2 saveCloudConfig call sites in cloud_test.go
- 4 saveCloudConfig call sites in sync_cloud_auth_test.go
all migrate to cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{...})
Updates two tests that pinned the legacy 'reject ?q=1'
behavior: TestCmdCloudStatusRejectsInvalidEffectiveRuntimeServerURL
-> TestCmdCloudStatusAcceptsAndClearsQueryInRuntimeServerURL
and TestStoreSyncStatusProviderRejectsInvalidRuntimeServerURL
-> TestStoreSyncStatusProviderAcceptsAndClearsQueryInRuntimeServerURL.
The new contracts assert that URLs with ?q=1 are accepted
and cleared (per spec REQ-1's deliberate spec change), not
rejected with 'query is not allowed'.
Deletes the 4 TestDefaultCloudDaemonProbe* tests from
cmd/engram/cloud_daemon_probe_test.go; their coverage is
already provided by internal/cloudconfig/daemon_test.go
(T-608.4). The remaining test file keeps
TestResolveDaemonProbePortHonorsEnvAndDefaults and the
rewritten TestPrintCloudStatusDaemonProbeFormatsEachState.
The net diff for this commit is -80 lines (186 added, 266
deleted across 8 files), demonstrating the diff-shrink the
spec forecast for the cleanup pass. The CLI migration
(Phase 4) is now COMPLETE: T-608.6 through T-608.15 have
all landed.
The next phase is TUI migration (Phase 5, T-608.16 to
T-608.20).
Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
…-608.16)
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR).
Deletes the forked cloudConfigPath, loadCloudConfig, saveCloudConfig,
tokenSourceMessage, effectiveCloudToken helpers from internal/tui/cloud.go.
Replaces with calls to internal/cloudconfig and two small TUI-side
helpers that restore parity with the CLI:
- tuiCloudConfigForUI(dataDir) reads cloudconfig.Load and applies
ENGRAM_CLOUD_SERVER as an override (whitespace-only env is
treated as unset), closing the silent precedence drift the
forked effectiveCloudToken had versus the CLI's
resolveCloudRuntimeConfig.
- saveTUIServerURL(dataDir, url) loads, sets ServerURL, saves
through cloudconfig.Save — preserving any pre-existing Token
field, the same contract the old forked saveCloudConfig had.
loadCloudConfigCmd in model.go now uses tuiCloudConfigForUI plus
cloudconfig.SourceLabel(cloudconfig.EffectiveToken(dataDir)) so the
view-layer TokenSource* constants still mirror the canonical labels.
The save branch in update.go routes through saveTUIServerURL and
validates the input with cloudconfig.ValidateServerURL.
validateCloudServerURL is kept as a one-line shim for now; T-608.17
will delete it once pingCloudServerStatus is verified to use
cloudconfig.ValidateServerURL directly.
Tests cover: env precedence, whitespace handling, file missing,
malformed JSON, env-only, env-overrides-file, all three token
sources, save preserves token, save propagates malformed error,
ping tests, and the validateCloudServerURL shim contract.
Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
…08.17)
Strict TDD cycle (RED-GREEN-TRIANGULATE-REFACTOR).
Adds cloudDaemonProbeMsg{result cloudconfig.Result, err error} and
probeLocalDaemonCmd() tea.Cmd that calls cloudconfig.LocalDaemonProbe
with cloudconfig.ResolvePort (which honors ENGRAM_PORT). The message
and command are the TUI's entry point for the local-daemon probe;
T-608.19 wires them into the Cloud Status update arm so the status
header and the local-daemon line populate INDEPENDENTLY (the
decoupling fix per the tui-probe-bundle observation in #1449).
Deletes the validateCloudServerURL shim added in T-608.16. The TUI
now uses cloudconfig.ValidateServerURL directly inside
pingCloudServerStatus. The behavior change per spec REQ-1: URLs
with ?q=1 or #frag are accepted and the query/fragment is cleared
on success. Tests pin the new accept-and-clear contract:
TestPingCloudServerAcceptsAndClearsQueryInURL,
TestPingCloudServerAcceptsAndClearsFragmentInURL, plus the existing
bad-scheme / empty-host rejection paths.
The probe and the cloud /health ping (pingCloudServer) are kept
distinct per ADR-2: they have different timeouts (1s vs 3s) and
different status mappings (ProbeRunning/NotRunning/Unreachable vs
reachable/unauthorized/unreachable). The cloudconfig package does
not need to own pingCloudServer.
Tests cover the new probe message (3 status outcomes), the new
URL accept-and-clear contract (4 cases), and exercise the existing
pingCloudServer branches unchanged.
Part of Gentleman-Programming#577, part of tui-cloud-config-remediation chain.
Contributor
Author
|
Hi @Alan-TheGentleman! Status update on the tui-cloud-config-remediation work. What I shipped to PR #608 (just pushed, body updated):
Behavior changes (deliberate per spec REQ-1):
PR diff: 5183 lines (over the 800-line review budget — flagged in the PR description). 30 commits, 24 packages green. What I deferred (T-608.18-20 in the original plan): the TUI status view migration. This needs the Cloud Status view code that lives on My proposed path forward:
Two questions for you:
Thanks for the review and the rebase-against-main direction — that worked smoothly after the initial cross-fork blocker. CC: #577 |
Contributor
Author
|
Final note: the PR came out much bigger than expected (5183 lines vs the 800-line review budget). I had to absorb the package extraction, CLI migration, and TUI form migration into a single PR after the rebase-against-main strategy simplified the chain. Despite the size, the requested work from your "Third, the design item" review is done and verified:
T-608.18-20 (the TUI status view migration that needs the code on |
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
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
This PR delivers the bulk of Alan's "Third, the design item" review feedback for the TUI cloud config chain:
cloud.jsonis persisted by the CLI or by the user's hand.internal/cloudconfigshared package — extracted from the duplicated code ininternal/tui/cloud.goandcmd/engram/cloud.go. This eliminates the drift risk Alan flagged.cmd/engram/cloud.go(cloudConfigPath,loadCloudConfig,saveCloudConfig,validateCloudServerURL); 23 call sites migrated tocloudconfig.*.tuiCloudConfigForUIwhich applies theENGRAM_CLOUD_SERVERenv override (parity with CLI; fixes the silentENGRAM_CLOUD_SERVERprecedence drift).cloudDaemonProbeMsgandprobeLocalDaemonCmdadded so the local-daemon probe is independent from the status header (decoupling, not unblocking the event loop).What's included
Package extraction (5 commits):
454729cfeat(cloudconfig): add internal/cloudconfig package with config IO5f670e0feat(cloudconfig): add EffectiveToken with file/env precedencec4f949dfeat(cloudconfig): add ValidateServerURLa41ec30feat(cloudconfig): add LocalDaemonProbe with timeout var seamd77e66dfeat(cloudconfig): add TUI/CLI token source agreement testCLI migration (10 commits):
3390d71refactor(cli): migrate cloud.go:342 to cloudconfig.ValidateServerURLb222342refactor(cli): migrate cloud.go:496 (cloudUpgradeBootstrap) to cloudconfig.ValidateServerURLd065832refactor(cli): migrate cmdCloudConfig to cloudconfig7cb61d9refactor(cli): migrate snapshot writeback to cloudconfig.Pathfc3e81atest(cli): pin cmdCloudStatus nil-vs-zero-value contract72eb3e0refactor(cli): migrate resolveCloudRuntimeConfig to cloudconfiga0184cftest(cli): audit autosync caller of resolveCloudRuntimeConfiga9f0e30refactor(cli): make cloud_daemon_probe.go a thin wrapper343e620refactor(cli): port daemon probe test + delete legacy helpersTUI migration (partial, 2 commits):
cd6971arefactor(tui): delete forked cloud config helpers, use cloudconfig38f9128refactor(tui): split local-daemon probe into independent tea.CmdRebased history (preserved, rebase-against-
main):Behavior changes (per spec REQ-1)
ENGRAM_CLOUD_SERVERenv override: previously dropped silently in the TUI'seffectiveCloudToken; now applied (parity with CLI).?q=1/#fragin URLs: previously REJECTED by the legacyvalidateCloudServerURL; now ACCEPTED and CLEARED (URL is cleaned to base). This is a deliberate spec change consistent with how the TUI's view layer consumes the URL.cloud.jsonfile mode: now normalized to0o644on every save (defensive; previously only set on creation).engram cloud statusoutput: byte-identical to pre-change (per the regression contract).What's NOT in this PR (deferred)
cloudDaemonProbeMsgintocloudStatusLoadedMsg, addm.CloudDaemonProbe *cloudconfig.Resultto the model, render the daemon line from the new field. This work needs the Cloud Status view code that lives onfeat/tui-cloud-config-06-status-parity(PR feat(tui): add CLI parity fields to cloud status view #612's branch). Bundling both branches in one rebase was impractical due to ~9 conflict files per commit × 20 commits. Plan: open a separate PR for T-608.18-20 that targetsfeat/tui-cloud-config-06-status-parityonce this PR lands.0o600file permissions, trailing-slash URL normalization, nil-store guards, error rendering, async enrollment write, view-bound rendering: out of scope per proposal #1442.Test evidence
go test ./...green across all 24 packagesgo test -cover ./internal/cloudconfig/...→ 92.9% coverage on the new packagego test -cover ./internal/tui/...→ maintainedgo test -cover ./cmd/engram/...→ maintainedgo test -race ./internal/...→ no data racesTestCLIAndTUIAgreeOnTokenSourcepins the CLI/TUI agreement on token-source labels (catches theENGRAM_CLOUD_SERVERdrift).Migration risks (per design #1447)
engram cloud statusproduces identical output pre/post change (verified byTestCloudStatusNoFileNoEnvReportsNotConfiguredand related tests).ENGRAM_CLOUD_SERVERprecedence: the old TUIeffectiveCloudTokenwas missing this override; the migration fixes it. The TUI'stuiCloudConfigForUIhelper applies the override (whitespace-only env treated as unset). AllTestTuiCloudConfigForUI*tests pin the new contract.Merge order
This is PR 2 of 6 in the
tui-cloud-configfeature delivery. Depends on PR 1 (merged first). After PR 1 is merged, please click "Update branch" on this PR so the diff shows only this PR's changes (the rebase-against-mainhas already been done locally, but GitHub may need a refresh).Order:
Issue link
Part of #577