feat(prompt): shell backtick substitution in PROMPT1/PROMPT2#748
feat(prompt): shell backtick substitution in PROMPT1/PROMPT2#748
Conversation
REV Review — fix/744-prompt-backtickAPPROVE Clean, correct implementation matching psql semantics.
Minor nit: No SOC2 items. No new dependencies. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #748 +/- ##
==========================================
+ Coverage 69.11% 69.17% +0.06%
==========================================
Files 47 47
Lines 32000 32062 +62
==========================================
+ Hits 22115 22177 +62
Misses 9885 9885 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review: PR #748 — feat(prompt): shell backtick substitution in PROMPT1/PROMPT2Reviewer: Max SummaryAdds psql-compatible backtick command substitution to BLOCKINGB1 — Unterminated backtick silently drops content | Confidence: 8If the prompt string contains an unmatched opening backtick (e.g. // Current: no check for unclosed backtick
for inner in chars.by_ref() {
if inner == '`' { break; }
cmd.push(inner);
}
// If loop exits without finding '`', cmd contains the rest of the string
// and gets executed as a shell command.Fix: Track whether the closing backtick was found. If not, emit the opening backtick and the collected characters as-is (same as bash/psql behavior for unmatched backticks in non-interactive contexts — silently pass through or error). let mut found_close = false;
for inner in chars.by_ref() {
if inner == '`' { found_close = true; break; }
cmd.push(inner);
}
if !found_close {
result.push('`');
result.push_str(&cmd);
continue; // or break, depending on desired semantics
}B2 —
|
|
Fixed REV B1: unterminated backtick now passes through literally instead of executing as shell command. Added test |
REV Code Review Report
BLOCKING ISSUES (1)CRITICAL
NON-BLOCKING (0)No non-blocking issues found. POTENTIAL ISSUES (2)MEDIUM
LOW
Summary
Note:
REV-assisted review (AI analysis by postgres-ai/rev) |
REV Code Review — PR #748PR: #748 — feat(prompt): shell backtick substitution in PROMPT1/PROMPT2 BLOCKING ISSUESHIGH
NON-BLOCKINGLOW
LOW
POTENTIAL ISSUESMEDIUM
MEDIUM
Summary
6 unit tests cover the main scenarios. The HIGH finding is not a runtime bug — the feature works as intended — but the shell execution behavior is undocumented and security-relevant. CI is green. REV-assisted review (AI analysis by postgres-ai/rev) |
|
REV PR #748 — feat(prompt): shell backtick substitution in PROMPT1/PROMPT2 Summary: Adds Observations:
Verdict: Correct implementation, tests cover all edge cases. Points 1 and 6 worth a quick look before merge but neither is a blocker. |
|
REV #748 — feat(prompt): shell backtick substitution in PROMPT1/PROMPT2Solid. Matches psql behavior, good test coverage, handles edge cases cleanly. What's good
Concerns
Minor
Verdict: approve with nits. Items 3 and 4 are worth a quick fix; 1 and 2 are acceptable known limitations. |
REV Code Review — PR #748: feat(prompt): shell backtick substitution in PROMPT1/PROMPT2CI: green | Diff scope: Blocking findingsNone. Non-blocking findings[SECURITY / MEDIUM] Arbitrary shell command execution via PROMPT1/PROMPT2
[BUG / LOW] Unterminated backtick handling emits the backtick but drops the command content result.push('`');
result.push_str(&cmd);
continue;The test [BEHAVIOR / LOW] Trailing newlines: only output.trim_end_matches('\n').trim_end_matches('\r')This strips any number of trailing [STYLE / LOW] Using Potential issues (low confidence)[BEHAVIOR] Nested backticks are not supported Backtick nesting (e.g., SummaryMinimal, correct implementation that matches psql semantics. Test coverage covers the key cases. The security note about shell execution from config files is the main item worth documenting. No blockers. |
REV Code Review — PR #748 (re-review after fixes)Branch: SECURITY REVIEWERFinding 1 Finding 2 No other security findings. Shell commands come exclusively from user-set BUG HUNTERFinding 3 No other bugs. Logic flow, iterator usage, TEST ANALYZERFinding 4 Finding 5 Coverage is otherwise solid: happy path, multi-expand, unterminated backtick, failing command, no-backtick passthrough. Good test naming and isolation. GUIDELINES CHECKERFinding 6 Commit messages comply with Conventional Commits. All 4 commits are well-scoped: DOCS REVIEWERFinding 7 The SPEC.md changes are accurate and appropriately scoped — backtick behavior documented in both FR-2 (feature) and the future TODO checklist. Summary
BLOCKING findings: NO All findings are LOW/MEDIUM/INFO. The primary fix worth making before merge is Finding 3 (newline trim order — easy one-liner) combined with Finding 1 ( The core implementation is correct, psql-compatible, well-documented, and the unterminated-backtick fix from the previous review is properly handled. PASS. REV automated review — agents: Security (Opus), Bug Hunter (Opus), Test Analyzer (Sonnet), Guidelines (Sonnet), Docs (Sonnet) |
|
CI is green and REV review found no blocking findings. This PR is ready for merge — awaiting Nik's approval. |
|
REV PR #748 — feat(prompt): shell backtick substitution in PROMPT1/PROMPT2 Clean implementation, matches psql behavior, good test coverage. Observations:
No blockers. The timeout concern (#1) is a quality-of-life issue, not a correctness issue. |
Testing Evidence — Shell Backtick Substitution in PROMPT1/PROMPT2Branch: BuildTest 1: PROMPT1 with
|
Add regression tests confirming that content after an unterminated backtick is not silently dropped. The existing expand_prompt_backticks() implementation already handles this correctly via the found_close guard — but only one test existed for the unterminated case (`unclosed), which did not cover the actual reported bug pattern: 'hello `date world' where 'world' would be lost. Two new tests added: - backtick_unterminated_preserves_content_after: the exact bug from #744 - backtick_unterminated_with_prefix_and_suffix: prefix + suffix variant
|
CI is green and REV review found no blocking findings. Testing evidence posted. This PR is ready for merge — awaiting Nik's approval. |
Closes #744
Implements backtick command substitution in PROMPT1/PROMPT2, matching psql behaviour.
Example:
PROMPT1='hostname:%n@%m:%/> 'now renders with actual hostname output.Tests added for: valid command, no trailing newline, failing command (empty), no backticks, multiple substitutions.