Hardening + 10x discovery: security audit changeset#7
Merged
Conversation
Discovery now scans providers concurrently and parses session files with a bounded worker pool; Claude and Codex files are read in a single head+tail pass instead of up to three full scans. On a real store with 716 sessions listing drops from ~14s to ~1.4s with a byte-identical ID set. Conversions previously ran transcript text through the preview cleaner, which folds all whitespace — flattening code blocks and newlines. Text handling is now split three ways: cleanTranscriptText preserves layout (and secret values) for native conversion, cleanPreviewText folds and redacts for the picker, and RedactSecrets covers credential patterns (passwords, API keys, bearer/JWT, PEM blocks, known token shapes) without touching layout. Also: converted files are written 0600 (dirs 0700) with a parent-dir fsync after rename; deterministic sort tiebreakers; deletes now clean Claude sessions-index.json entries (best-effort — a corrupt index never blocks a delete) and jcode .bak/.journal sidecars; discovery output is stripped of ANSI/OSC and bidi controls at the parsing boundary. Based on the 2026-07-09 codex audit; review adjusted the Claude index cleanup from fail-closed to best-effort. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_transcript now redacts credential-like values before returning text to a model and labels every transcript as untrusted historical data; a tool call cannot bypass this. Users who need verbatim values opt in by starting the server with --allow-secrets. --read-only registers only the non-writing tools. max_turns gains a hard cap of 500. The updater gets response-size limits, an HTTP timeout, extraction size caps, and managed-install detection that points brew/go installs at the right upgrade command instead of overwriting the binary in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Session metadata and previews pass through SafeDisplayText before hitting the terminal, closing ANSI/OSC escape and bidi-override injection via crafted session content. Delete becomes an async command behind the existing busy guard, matching branch/convert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI enforces 80% total / 75% per-package coverage with -shuffle, actionlint, shellcheck and govulncheck, and adds linux/arm64 to the cross-compile matrix. Releases verify the tag format and that the tagged commit is an ancestor of main before building. Adds CONTRIBUTING, SECURITY, CODE_OF_CONDUCT, CODEOWNERS, issue/PR templates and Dependabot config; README documents the trust model for MCP transcripts and the new flags, and the real-CLI e2e script grows opencode/gemini seeding with quota-aware skips. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds repository governance and CI controls, hardens session discovery, redaction, storage, deletion, MCP access, update installation, and terminal rendering, and expands unit and end-to-end validation across providers and platforms. ChangesRepository and delivery controls
Runtime hardening
Installation and interfaces
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🧹 Nitpick comments (2)
.github/pull_request_template.md (1)
5-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the vulnerability scan to the checklist.
CI now gates on vulnerability scanning, but the template only asks for test/vet/lint. Adding that step here keeps the PR checklist aligned with the new hardening gates.
🤖 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 @.github/pull_request_template.md around lines 5 - 10, The PR checklist under the Verification section is missing the new vulnerability scan gate, so update the checklist in the pull request template to include the vulnerability scanning step alongside `go test`, `go vet`, and `golangci-lint`. Keep the existing checklist structure in `.github/pull_request_template.md` and add the scan item so the template reflects the current CI hardening requirements.internal/tui/tui.go (1)
1163-1175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
th.detail.GetFrameSize()instead of hardcoding2forinnerWidth.
compoundViewcomputesinnerWidth := max(1, boxWidth-2), assuming the detail style's frame is exactly 2 cells (1-cell border per side).detailView(line 949) correctly queriesth.detail.GetFrameSize(). If the style ever adds padding,compoundViewwould truncate lines to a width wider than the actual text area, causing lipgloss to further clip content unnecessarily.♻️ Proposed fix for consistent frame size calculation
boxWidth := 64 if m.width > 0 { boxWidth = max(1, min(m.width, 64)) } - innerWidth := max(1, boxWidth-2) + frameW, _ := th.detail.GetFrameSize() + innerWidth := max(1, boxWidth-frameW) for index := range lines { lines[index] = truncateCells(lines[index], innerWidth) }🤖 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/tui.go` around lines 1163 - 1175, The inner width calculation in compoundView is hardcoding the detail frame size, so update it to use th.detail.GetFrameSize() just like detailView does. Locate the compoundView rendering logic and replace the fixed subtraction in innerWidth with the actual frame size from th.detail so truncation matches the rendered content area even if the style changes.
🤖 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 @.github/workflows/ci.yml:
- Around line 34-47: The per-package coverage enforcement in the CI workflow is
only checking `go test` output lines matched by the `sed` pipeline, so packages
with no test files are skipped and can bypass the 75% floor. Update the coverage
check in the workflow step that uses `go list ./...`, `go test`, and the
`sed`/`awk` loop so it explicitly compares all listed packages against the
packages present in `test-output.txt` and fails if any package is missing a
coverage entry. Ensure the logic still reports coverage per package and uses the
existing `coverage.out`/`test-output.txt` flow.
In `@cmd/showagent/main.go`:
- Line 412: The command rendering in the showagent output is over-sanitizing
`recipe.CommandString` by passing it through `session.SafeDisplayText`, which
can rewrite copy-pasteable shell commands when quoted arguments contain spaces.
Update the `fmt.Fprintf` call in `main.go` to use a control-stripping-only
display path for `recipe.CommandString` instead of `SafeDisplayText`, and keep
the existing surrounding command formatting unchanged.
In `@cmd/showagent/update.go`:
- Line 36: The shared httpClient in update.go is using a blanket 30s Timeout
that can cancel long archive/body downloads in installRelease and downloadBytes
even when the request context allows more time. Remove the client-wide Timeout
from the shared client and let the existing context control the install path; if
needed, move any safety limits to transport-level settings instead. Use the
httpClient variable and the downloadBytes/installRelease flow as the places to
update.
In `@internal/session/jcode.go`:
- Around line 57-69: The deletion logic in the session cleanup path does not
match the current comment, since the loop in the jcode removal flow still
attempts to delete row.File even after sidecar failures while only aggregating
errors. Update the comment near the removal sequence to describe the actual
best-effort behavior in this path: it removes all known session artifacts, skips
missing files, and collects non-NotExist failures for reporting rather than
implying the canonical session is preserved on partial failure. Use the
identifiers base, row.File, removed, and failures to locate the block.
In `@README.md`:
- Around line 36-37: The README’s “100% local” claim overstates the behavior
given the updater/setup sections still imply networked activity; revise the
marketing copy to qualify it as “sessions stay local by default” or similar.
Update the wording in the main feature blurb and any matching phrasing elsewhere
in README so it stays consistent with the implementation.
---
Nitpick comments:
In @.github/pull_request_template.md:
- Around line 5-10: The PR checklist under the Verification section is missing
the new vulnerability scan gate, so update the checklist in the pull request
template to include the vulnerability scanning step alongside `go test`, `go
vet`, and `golangci-lint`. Keep the existing checklist structure in
`.github/pull_request_template.md` and add the scan item so the template
reflects the current CI hardening requirements.
In `@internal/tui/tui.go`:
- Around line 1163-1175: The inner width calculation in compoundView is
hardcoding the detail frame size, so update it to use th.detail.GetFrameSize()
just like detailView does. Locate the compoundView rendering logic and replace
the fixed subtraction in innerWidth with the actual frame size from th.detail so
truncation matches the rendered content area even if the style changes.
🪄 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: 593ba566-90b3-46ee-aa6c-2a2c2f38b328
📒 Files selected for processing (42)
.github/CODEOWNERS.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/dependabot.yml.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/release.ymlCODE_OF_CONDUCT.mdCONTRIBUTING.mdREADME.mdSECURITY.mdcmd/showagent/main.gocmd/showagent/main_test.gocmd/showagent/replace_unix.gocmd/showagent/replace_windows.gocmd/showagent/update.gocmd/showagent/update_test.godemo/fixtures/gen.shgo.modinternal/mcpserver/server.gointernal/mcpserver/server_test.gointernal/session/claude.gointernal/session/codex.gointernal/session/compound.gointernal/session/compound_plugin.gointernal/session/compound_plugin_test.gointernal/session/e2e_real_test.gointernal/session/gemini.gointernal/session/handoff.gointernal/session/hardening_test.gointernal/session/jcode.gointernal/session/opencode.gointernal/session/opencode_test.gointernal/session/session.gointernal/session/session_test.gointernal/tui/table.gointernal/tui/table_test.gointernal/tui/tui.gointernal/tui/tui_test.goscripts/e2e-real.shscripts/install.sh
Comment on lines
+34
to
+47
| - name: Enforce coverage floors (total 80%, per package 75%) | ||
| # The per-package floor sits below the total floor on purpose: several | ||
| # packages hover just above 80%, and a floor equal to the current | ||
| # value turns any small refactor into a red build. | ||
| run: | | ||
| set -eu | ||
| total="$(go tool cover -func=coverage.out | awk '$1 == "total:" { gsub("%", "", $3); print $3 }')" | ||
| test -n "$total" | ||
| printf 'total coverage: %s%%\n' "$total" | ||
| awk -v total="$total" 'BEGIN { exit !(total + 0 >= 80) }' | ||
| sed -n 's/.*coverage: \([0-9][0-9.]*\)%.*/\1/p' test-output.txt | while IFS= read -r package_coverage; do | ||
| printf 'package coverage: %s%%\n' "$package_coverage" | ||
| awk -v coverage="$package_coverage" 'BEGIN { exit !(coverage + 0 >= 75) }' | ||
| done |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Per-package coverage floor can be silently bypassed by packages with no test files.
The sed extraction at line 44 only matches lines containing coverage: from go test output. Packages without any test files produce no coverage line, so they are silently excluded from the per-package 75% floor check. This means a package could regress to 0% coverage without failing CI.
Consider adding an explicit check for packages that appear in go list ./... but are missing from the coverage output.
🛡️ Suggested addition to detect untested packages
- name: Enforce coverage floors (total 80%, per package 75%)
# The per-package floor sits below the total floor on purpose: several
# packages hover just above 80%, and a floor equal to the current
# value turns any small refactor into a red build.
run: |
set -eu
total="$(go tool cover -func=coverage.out | awk '$1 == "total:" { gsub("%", "", $3); print $3 }')"
test -n "$total"
printf 'total coverage: %s%%\n' "$total"
awk -v total="$total" 'BEGIN { exit !(total + 0 >= 80) }'
+ # Fail if any package with test files is missing from coverage output
+ covered="$(sed -n 's/^\(ok\|FAIL\)\s\+\S\+.*coverage: \([0-9][0-9.]*\)%.*/\1/p' test-output.txt | sort -u)"
sed -n 's/.*coverage: \([0-9][0-9.]*\)%.*/\1/p' test-output.txt | while IFS= read -r package_coverage; do
printf 'package coverage: %s%%\n' "$package_coverage"
awk -v coverage="$package_coverage" 'BEGIN { exit !(coverage + 0 >= 75) }'
done🤖 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 @.github/workflows/ci.yml around lines 34 - 47, The per-package coverage
enforcement in the CI workflow is only checking `go test` output lines matched
by the `sed` pipeline, so packages with no test files are skipped and can bypass
the 75% floor. Update the coverage check in the workflow step that uses `go list
./...`, `go test`, and the `sed`/`awk` loop so it explicitly compares all listed
packages against the packages present in `test-output.txt` and fails if any
package is missing a coverage entry. Ensure the logic still reports coverage per
package and uses the existing `coverage.out`/`test-output.txt` flow.
| _, _ = fmt.Fprintf(w, " cwd: %s\n", emptyFallback(recipe.CWD)) | ||
| _, _ = fmt.Fprintf(w, " storage: %s\n", recipe.StorageLocation) | ||
| _, _ = fmt.Fprintf(w, " session: %s\n", session.SafeDisplayText(recipe.ID)) | ||
| _, _ = fmt.Fprintf(w, " command: %s\n", session.SafeDisplayText(recipe.CommandString)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect stripTerminalControls and how CommandString is constructed.
rg -nP -C3 'func stripTerminalControls' internal/session
rg -nP -C3 'CommandString' internal/sessionRepository: aytzey/showagent
Length of output: 2297
🏁 Script executed:
#!/bin/bash
sed -n '330,380p' internal/session/session.go
printf '\n----\n'
sed -n '1,130p' internal/session/recipe.go
printf '\n----\n'
sed -n '390,430p' cmd/showagent/main.goRepository: aytzey/showagent
Length of output: 7345
🏁 Script executed:
#!/bin/bash
sed -n '380,470p' internal/session/recipe.go
printf '\n----\n'
sed -n '1,220p' internal/session/session.go
printf '\n----\n'
rg -nP -C2 'SafeDisplayText\(' internal/sessionRepository: aytzey/showagent
Length of output: 6745
🏁 Script executed:
#!/bin/bash
sed -n '380,520p' internal/session/recipe.go
printf '\n----\n'
sed -n '300,330p' internal/session/session.go
printf '\n----\n'
sed -n '1,140p' internal/session/session_test.goRepository: aytzey/showagent
Length of output: 8372
🏁 Script executed:
#!/bin/bash
ast-grep outline internal/session/recipe.go --view expanded
printf '\n----\n'
rg -n -C3 'func ShellCommand|ResumeArgs|shell' internal/sessionRepository: aytzey/showagent
Length of output: 9566
recipe.CommandString should skip SafeDisplayText.
SafeDisplayText folds whitespace, so a copy-pasteable shell command can be rewritten into a different command when an argument contains spaces inside quotes. Use a control-stripping-only path here.
🤖 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/showagent/main.go` at line 412, The command rendering in the showagent
output is over-sanitizing `recipe.CommandString` by passing it through
`session.SafeDisplayText`, which can rewrite copy-pasteable shell commands when
quoted arguments contain spaces. Update the `fmt.Fprintf` call in `main.go` to
use a control-stripping-only display path for `recipe.CommandString` instead of
`SafeDisplayText`, and keep the existing surrounding command formatting
unchanged.
| latestReleaseURL = defaultLatestReleaseURL | ||
| releaseBaseURL = defaultReleaseBaseURL | ||
| httpClient = http.DefaultClient | ||
| httpClient = &http.Client{Timeout: 30 * time.Second} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding functions.
ast-grep outline cmd/showagent/update.go --view expanded || true
echo '---'
wc -l cmd/showagent/update.go
echo '---'
sed -n '1,240p' cmd/showagent/update.go
echo '---'
sed -n '240,520p' cmd/showagent/update.go
echo '---'
# Find where httpClient is used and where contexts are created/passed.
rg -n "httpClient|fetchLatestRelease|downloadBytes|installRelease|context.WithTimeout|WithTimeout\\(" cmd/showagent -SRepository: aytzey/showagent
Length of output: 19381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any scripts/install.sh mention from the comment, if present.
fd -a 'install.sh|update.go' . || true
echo '---'
rg -n "max-time|retry|curl|wget|install.sh" -S . || trueRepository: aytzey/showagent
Length of output: 1901
30s client timeout can abort archive downloads http.Client.Timeout caps the entire body read, so the shared client will still fail downloadBytes during installRelease after 30s even though that path already uses a 5-minute context. Drop the blanket client timeout and let the request context bound the install path; add transport-level connect/TLS/header timeouts only if needed.
🤖 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/showagent/update.go` at line 36, The shared httpClient in update.go is
using a blanket 30s Timeout that can cancel long archive/body downloads in
installRelease and downloadBytes even when the request context allows more time.
Remove the client-wide Timeout from the shared client and let the existing
context control the install path; if needed, move any safety limits to
transport-level settings instead. Use the httpClient variable and the
downloadBytes/installRelease flow as the places to update.
Comment on lines
+57
to
+69
| // Remove derivative copies first and the canonical session last. If a | ||
| // sidecar cannot be removed, the main session remains discoverable instead | ||
| // of leaving the picker with a stale row after a partial delete. | ||
| paths := []string{base + ".bak", base + ".journal.jsonl", row.File} | ||
| removed := false | ||
| var failures []error | ||
| for _, path := range paths { | ||
| if err := os.Remove(path); err == nil { | ||
| removed = true | ||
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| failures = append(failures, fmt.Errorf("remove %s: %w", path, err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Delete comment doesn't match implementation.
The comment states "If a sidecar cannot be removed, the main session remains discoverable," but the loop continues to remove row.File even when a sidecar removal fails with a non-NotExist error. The current best-effort behavior is reasonable — it cleans up as much as possible and aggregates failures — but the comment misleads future readers about the partial-delete semantics.
📝 Proposed comment fix
base := strings.TrimSuffix(row.File, ".json")
- // Remove derivative copies first and the canonical session last. If a
- // sidecar cannot be removed, the main session remains discoverable instead
- // of leaving the picker with a stale row after a partial delete.
+ // Remove derivative copies first and the canonical session last, so
+ // that a partial delete cleans up as many artifacts as possible. Any
+ // non-NotExist removal errors are aggregated and returned; if nothing
+ // was removed, the session is reported as not found.
paths := []string{base + ".bak", base + ".journal.jsonl", row.File}📝 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
| // Remove derivative copies first and the canonical session last. If a | |
| // sidecar cannot be removed, the main session remains discoverable instead | |
| // of leaving the picker with a stale row after a partial delete. | |
| paths := []string{base + ".bak", base + ".journal.jsonl", row.File} | |
| removed := false | |
| var failures []error | |
| for _, path := range paths { | |
| if err := os.Remove(path); err == nil { | |
| removed = true | |
| } else if !errors.Is(err, os.ErrNotExist) { | |
| failures = append(failures, fmt.Errorf("remove %s: %w", path, err)) | |
| } | |
| } | |
| // Remove derivative copies first and the canonical session last, so | |
| // that a partial delete cleans up as many artifacts as possible. Any | |
| // non-NotExist removal errors are aggregated and returned; if nothing | |
| // was removed, the session is reported as not found. | |
| paths := []string{base + ".bak", base + ".journal.jsonl", row.File} | |
| removed := false | |
| var failures []error | |
| for _, path := range paths { | |
| if err := os.Remove(path); err == nil { | |
| removed = true | |
| } else if !errors.Is(err, os.ErrNotExist) { | |
| failures = append(failures, fmt.Errorf("remove %s: %w", path, 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 `@internal/session/jcode.go` around lines 57 - 69, The deletion logic in the
session cleanup path does not match the current comment, since the loop in the
jcode removal flow still attempts to delete row.File even after sidecar failures
while only aggregating errors. Update the comment near the removal sequence to
describe the actual best-effort behavior in this path: it removes all known
session artifacts, skips missing files, and collects non-NotExist failures for
reporting rather than implying the canonical session is preserved on partial
failure. Use the identifiers base, row.File, removed, and failures to locate the
block.
Comment on lines
+36
to
+37
| - **100% local** — one static binary that reads your own files. No hosted | ||
| service, no telemetry, no account. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the “100% local” claim.
The later updater/setup sections still describe networked behavior, so this reads stronger than the implementation actually guarantees. Consider wording it as “sessions stay local by default” instead.
🤖 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 `@README.md` around lines 36 - 37, The README’s “100% local” claim overstates
the behavior given the updater/setup sections still imply networked activity;
revise the marketing copy to qualify it as “sessions stay local by default” or
similar. Update the wording in the main feature blurb and any matching phrasing
elsewhere in README so it stays consistent with the implementation.
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.
Full hardening pass from an external (Codex) security/quality audit of the codebase, reviewed change-by-change before merge.
Highlights
get_transcriptredacts credential-like values by default and labels transcripts as untrusted data;--allow-secretsopts out,--read-onlydrops the writing tools. A tool call cannot bypass redaction.sessions-index.jsonentries and jcode sidecars.Review adjustments made to the audit's version
Gates at HEAD: gofmt/vet clean, golangci-lint 0 issues,
go test ./... -racegreen, coverage 81.1% total (80.1/89.8/80.3/82.2 per package), real-store parity + timing verified.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation