feat(receipt): Exposure Receipt in qmax-code — LLM + cloud-API egress (QUA-1316)#149
Merged
Desperado merged 3 commits intoJul 13, 2026
Merged
Conversation
… (QUA-1316) Bring the Exposure Receipt to the qmax-code terminal agent so both customer-facing binaries emit receipts — making "Receipts, not promises" a platform property, not a CLI-only one. qmax-code has its own egress (LLM inference + internal/api cloud calls) that the qmax CLI's receipt does not cover; they are peers, not one wrapping the other. - internal/httpx: single egress chokepoint. Recording RoundTripper stream-hashes the request body (no buffering), records destination/category/size/SHA-256 — never content. NewClient + NewRequest replace all raw http.Client / http.NewRequest across the ~13 egress sites (api client, auth, codex/cc connect, anthropic + cerebras + ollama LLM calls, script fetch/update, vision, login, ollama probe). WithModel attributes the model on LLM entries. - internal/httpx/guard_test.go: static egress guard fails the build if any package outside httpx constructs a raw http client/request or imports a third-party HTTP lib. Wired into ci.yml via a new `go test` step (CI ran no tests before) — un-receipted egress can't merge. Verified by injection. - internal/exposure: qmax-code's own taxonomy (llm-prompt, llm-completion, cloud-api, mcp-traffic, control, uncategorized) + Classify. - Import shared schema github.com/Quality-Max/qmax-receipt v0.1.0 — one versioned contract, one receipt_version, no local copy. - Session-scoped receipt: NewCurrent at startup, finalized at exit (only if the session egressed). BaseDir = ~/.qmax-code keeps identity/store separate from the CLI's ~/.qamax. - `qmax-code receipt <list|show|verify>`: offline verify prints the provenance-not-honesty caveat. - release.yml stamps agent_build_sha via -X qmax-receipt.BuildSHA. End-to-end verified: a real session writes a signed ~/.qmax-code/receipts/<id>.json; `qmax-code receipt verify` validates it offline with correct category, hash and size, and no content. Parent: QUA-1315
Sigilix OverviewEffort: 4/5 (large) Quality gates
Summary — latest pushIntroduces the Exposure Receipt system for the qmax-code terminal agent, routing all ~13 LLM and cloud-API egress sites through a single Important files
Sequence diagramsequenceDiagram
participant CLI as qmax-code CLI
participant Httpx as httpx (Chokepoint)
participant Guard as Egress Guard (CI)
participant Receipt as Receipt Store
CLI->>Httpx: NewRequest / NewClient
Httpx->>Httpx: Record destination, category, size, SHA-256
Httpx->>Receipt: Append egress entry
CLI->>CLI: Session exit
CLI->>Receipt: Finalize & sign receipt
CLI->>CLI: receipt verify (offline)
Guard->>Guard: Scan module for forbidden symbols
Guard-->>Guard: Fail build on un-receipted egress
Confidence: 2/5The egress guard's directory-skip logic is flawed and will fail to skip nested sanctioned packages, and missing
Suggested labels:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
✅ QualityMax Pipeline
Powered by QualityMax — AI-Powered Test Automation |
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
|
|
||
| func receiptShow(idOrLatest string) { | ||
| r := mustResolveReceipt(idOrLatest) | ||
| data, _ := json.MarshalIndent(r, "", " ") |
There was a problem hiding this comment.
receiptShow silently produces empty output if JSON marshaling fails
The data, _ := json.MarshalIndent(r, "", " ") call discards the error. If marshaling fails (e.g., the receipt object contains an unsupported type), data will be nil, and fmt.Println(string(data)) prints a blank line with no error indication. This masks a corruption or schema mismatch. Check the error and print a diagnostic to stderr, then exit non-zero, so the user knows the receipt could not be displayed.
Example:
If the receipt module adds a non-marshalable field later, a user running `qmax-code receipt show <id>` will see a blank line instead of an error.
Suggested fix:
data, err := json.MarshalIndent(r, "", " "); if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err); os.Exit(1); }More Info
- Threat model: A silent failure in
receipt showcould lead users to believe a receipt is valid when it is not, undermining trust in the exposure receipt system's verifiability. - Specific code citations: cmd_receipt.go:68:
data, _ := json.MarshalIndent(r, "", " ")followed byfmt.Println(string(data))with no error check. - Existing protections: No error handling exists for JSON marshaling in receiptShow.
- Proposed mitigation: Check the error from json.MarshalIndent and exit with a non-zero code and error message on failure.
- Alternative mitigations considered: Accept the risk of silent failure, but this is inappropriate for a user-facing verification tool.
- Severity calibration: Medium impact: affects usability and trust in the receipt system, but does not create a security vulnerability or crash. The fix is trivial and prevents user confusion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd_receipt.go
Line: 65
Comment:
**receiptShow silently produces empty output if JSON marshaling fails**
The `data, _ := json.MarshalIndent(r, "", " ")` call discards the error. If marshaling fails (e.g., the receipt object contains an unsupported type), `data` will be nil, and `fmt.Println(string(data))` prints a blank line with no error indication. This masks a corruption or schema mismatch. Check the error and print a diagnostic to stderr, then exit non-zero, so the user knows the receipt could not be displayed.
Example:
If the receipt module adds a non-marshalable field later, a user running `qmax-code receipt show <id>` will see a blank line instead of an error.
Threat model:
A silent failure in `receipt show` could lead users to believe a receipt is valid when it is not, undermining trust in the exposure receipt system's verifiability.
Specific code citations:
cmd_receipt.go:68: `data, _ := json.MarshalIndent(r, "", " ")` followed by `fmt.Println(string(data))` with no error check.
Existing protections:
No error handling exists for JSON marshaling in receiptShow.
Proposed mitigation:
Check the error from json.MarshalIndent and exit with a non-zero code and error message on failure.
Alternative mitigations considered:
Accept the risk of silent failure, but this is inappropriate for a user-facing verification tool.
Severity calibration:
Medium impact: affects usability and trust in the receipt system, but does not create a security vulnerability or crash. The fix is trivial and prevents user confusion.
Suggested fix shape:
data, err := json.MarshalIndent(r, "", " "); if err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err); os.Exit(1); }
How can I resolve this? If you propose a fix, please make it concise.
| func (b *hashingBody) Close() error { return b.rc.Close() } | ||
|
|
||
| func (t *receiptTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| rec := receipt.FromContext(req.Context()) |
There was a problem hiding this comment.
receiptTransport forces a body on nil-body requests, altering HTTP semantics for GET endpoints
When the request has no body (req.Body == nil), the transport substitutes an empty hashingBody. This makes every request carry a body and a Content-Length: 0 header, including GET requests (e.g., /api/me, Ollama /api/tags, login polling). Strict servers that reject GET requests with a body may start refusing these calls. The original code sent these requests without a body; the transport should preserve nil-body semantics by recording a zero-size, empty-hash entry without modifying req.Body.
Example:
GET /api/tags on a strict reverse proxy that rejects GET-with-body → 400 response, Ollama probe fails.
Suggested fix:
if req.Body != nil { hb = &hashingBody{...}; req.Body = hb } else { hb = &hashingBody{rc: io.NopCloser(bytes.NewReader(nil)), h: sha256.New()} } // keep req.Body nilMore Info
- Threat model: An HTTP server that rejects a GET request with a body (non-conforming but existing) will return 400 to probes, login polls, or identity checks, breaking connectivity until the change is reverted.
- Specific code citations: httpx.go lines 100-108: the else branch assigns
req.Body = hbwhenreq.Body == nil, forcing a body on the request. - Existing protections: No guard exists; the transport always mutates req.Body.
- Proposed mitigation: When req.Body is nil, do not assign req.Body; compute a zero-length, empty-SHA entry directly without touching req.Body.
- Alternative mitigations considered: Accept the risk by documenting that all targeted servers tolerate GET with body — but this is fragile against future endpoint additions.
- Severity calibration: Medium impact: could break login/health checks against strict servers, but most modern servers accept it. Lower blast radius than a crash.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/httpx.go
Line: 106
Comment:
**receiptTransport forces a body on nil-body requests, altering HTTP semantics for GET endpoints**
When the request has no body (`req.Body == nil`), the transport substitutes an empty `hashingBody`. This makes every request carry a body and a `Content-Length: 0` header, including GET requests (e.g., `/api/me`, Ollama `/api/tags`, login polling). Strict servers that reject GET requests with a body may start refusing these calls. The original code sent these requests without a body; the transport should preserve nil-body semantics by recording a zero-size, empty-hash entry without modifying `req.Body`.
Example:
GET /api/tags on a strict reverse proxy that rejects GET-with-body → 400 response, Ollama probe fails.
Threat model:
An HTTP server that rejects a GET request with a body (non-conforming but existing) will return 400 to probes, login polls, or identity checks, breaking connectivity until the change is reverted.
Specific code citations:
httpx.go lines 100-108: the else branch assigns `req.Body = hb` when `req.Body == nil`, forcing a body on the request.
Existing protections:
No guard exists; the transport always mutates req.Body.
Proposed mitigation:
When req.Body is nil, do not assign req.Body; compute a zero-length, empty-SHA entry directly without touching req.Body.
Alternative mitigations considered:
Accept the risk by documenting that all targeted servers tolerate GET with body — but this is fragile against future endpoint additions.
Severity calibration:
Medium impact: could break login/health checks against strict servers, but most modern servers accept it. Lower blast radius than a crash.
Suggested fix shape:
if req.Body != nil { hb = &hashingBody{...}; req.Body = hb } else { hb = &hashingBody{rc: io.NopCloser(bytes.NewReader(nil)), h: sha256.New()} } // keep req.Body nil
How can I resolve this? If you propose a fix, please make it concise.
| // http.MethodPost, http.StatusOK, http.StatusText); only the egress-CREATING | ||
| // symbols below are forbidden. Route all outbound HTTP through | ||
| // httpx.NewClient / httpx.NewRequest. | ||
| func TestNoEgressOutsideHttpx(t *testing.T) { |
There was a problem hiding this comment.
Static egress guard test does not verify the injection scenario described in the PR body
The PR body states the guard was 'verified by injection' and includes an acceptance criterion: 'Inject a raw http.Client outside httpx → guard fails → revert'. However, the guard test TestNoEgressOutsideHttpx only scans for forbidden symbols; it does not contain a subtest that actually injects a raw client and asserts the test fails. This missing verification means the guard's effectiveness against real violations is not automatically validated in CI. The test should include a case that temporarily writes a forbidden Go file, runs the guard, and expects a failure, then cleans up.
Example:
The guard test passes today, but if the scanning logic had a bug (e.g., missing a symbol), it would still pass, allowing un-receipted egress.
Suggested fix:
func TestGuardFailsOnInjection(t *testing.T) {
tmpDir := t.TempDir()
badFile := filepath.Join(tmpDir, "bad.go")
os.WriteFile(badFile, []byte(`package bad
import "net/http"
var c = &http.Client{}`), 0644)
// Simulate guard scan on tmpDir and expect violation detection
}Why this wasn't caught: The existing test only scans the existing codebase; it does not test the guard's failure mode.
More Info
- Threat model: Without an injection test, regressions in the guard's detection logic could go unnoticed, allowing un-receipted egress to slip through CI.
- Specific code citations: PR body acceptance criteria: 'Inject a raw
http.Clientoutside httpx → guard fails → revert'. Guard test fileinternal/httpx/guard_test.gocontains only static scanning. - Existing protections: The guard test will fail if it finds violations in the codebase, but its own correctness is not tested.
- Proposed mitigation: Add a subtest
TestGuardFailsOnInjectionthat creates a temporary Go file with a rawhttp.Client{}construction, runs the guard logic, and expects a failure, then removes the file. - Alternative mitigations considered: Rely on manual verification, but that defeats the purpose of automated CI enforcement.
- Severity calibration: Score 4 because the guard is a critical enforcement mechanism for the 'Receipts, not promises' guarantee; a missing test for its core verification scenario is a significant coverage gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/guard_test.go
Line: 21
Comment:
**Static egress guard test does not verify the injection scenario described in the PR body**
The PR body states the guard was 'verified by injection' and includes an acceptance criterion: 'Inject a raw `http.Client` outside httpx → guard fails → revert'. However, the guard test `TestNoEgressOutsideHttpx` only scans for forbidden symbols; it does not contain a subtest that actually injects a raw client and asserts the test fails. This missing verification means the guard's effectiveness against real violations is not automatically validated in CI. The test should include a case that temporarily writes a forbidden Go file, runs the guard, and expects a failure, then cleans up.
Example:
The guard test passes today, but if the scanning logic had a bug (e.g., missing a symbol), it would still pass, allowing un-receipted egress.
Threat model:
Without an injection test, regressions in the guard's detection logic could go unnoticed, allowing un-receipted egress to slip through CI.
Specific code citations:
PR body acceptance criteria: 'Inject a raw `http.Client` outside httpx → guard fails → revert'. Guard test file `internal/httpx/guard_test.go` contains only static scanning.
Existing protections:
The guard test will fail if it finds violations in the codebase, but its own correctness is not tested.
Proposed mitigation:
Add a subtest `TestGuardFailsOnInjection` that creates a temporary Go file with a raw `http.Client{}` construction, runs the guard logic, and expects a failure, then removes the file.
Alternative mitigations considered:
Rely on manual verification, but that defeats the purpose of automated CI enforcement.
Severity calibration:
Score 4 because the guard is a critical enforcement mechanism for the 'Receipts, not promises' guarantee; a missing test for its core verification scenario is a significant coverage gap.
Suggested fix shape:
func TestGuardFailsOnInjection(t *testing.T) {
tmpDir := t.TempDir()
badFile := filepath.Join(tmpDir, "bad.go")
os.WriteFile(badFile, []byte(`package bad
import "net/http"
var c = &http.Client{}`), 0644)
// Simulate guard scan on tmpDir and expect violation detection
}
Why this wasn't caught:
The existing test only scans the existing codebase; it does not test the guard's failure mode.
How can I resolve this? If you propose a fix, please make it concise.
| case "verify": | ||
| receiptVerify(argAt(args, 1)) | ||
| default: | ||
| fmt.Fprintln(os.Stderr, "Usage: qmax-code receipt <list|show|verify> [id|latest]") |
There was a problem hiding this comment.
New CLI command module ships without a test
The file cmd_receipt.go implements the qmax-code receipt subcommand with three subcommands (list, show, verify) and helper functions. It contains non-trivial logic for parsing arguments, resolving receipt IDs, loading and displaying receipts, and error handling. The PR adds this as a new customer-facing surface but does not include a sibling test file. Without tests, regressions in argument parsing, error paths, or the 'latest' resolution logic could silently break the command.
Example:
If `mustResolveReceipt` incorrectly handles an empty ID, the command may crash or misbehave.
Suggested fix:
func TestReceiptList_NoReceipts(t *testing.T) {
// mock receipt.List to return empty slice, capture output, assert message
}
func TestReceiptVerify_InvalidID(t *testing.T) {
// expect exit code 1 and error message
}Why this wasn't caught: The source file has no sibling test in this PR.
More Info
- Threat model: A broken receipt command would undermine user trust in the exposure receipt system, as users rely on it to verify egress.
- Specific code citations: New file
cmd_receipt.gowith functionshandleReceiptCommand,receiptList,receiptShow,receiptVerify,mustResolveReceipt. - Existing protections: No test file exists for this module in the PR.
- Proposed mitigation: Create
cmd_receipt_test.gowith unit tests for each subcommand, including edge cases (no receipts, invalid ID, 'latest' resolution, error paths). - Alternative mitigations considered: Rely on integration tests elsewhere, but unit tests are more targeted and maintainable.
- Severity calibration: Score 4 because the command is a user-facing feature critical to the receipt system; missing tests increase risk of regression.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd_receipt.go
Line: 31
Comment:
**New CLI command module ships without a test**
The file `cmd_receipt.go` implements the `qmax-code receipt` subcommand with three subcommands (`list`, `show`, `verify`) and helper functions. It contains non-trivial logic for parsing arguments, resolving receipt IDs, loading and displaying receipts, and error handling. The PR adds this as a new customer-facing surface but does not include a sibling test file. Without tests, regressions in argument parsing, error paths, or the 'latest' resolution logic could silently break the command.
Example:
If `mustResolveReceipt` incorrectly handles an empty ID, the command may crash or misbehave.
Threat model:
A broken receipt command would undermine user trust in the exposure receipt system, as users rely on it to verify egress.
Specific code citations:
New file `cmd_receipt.go` with functions `handleReceiptCommand`, `receiptList`, `receiptShow`, `receiptVerify`, `mustResolveReceipt`.
Existing protections:
No test file exists for this module in the PR.
Proposed mitigation:
Create `cmd_receipt_test.go` with unit tests for each subcommand, including edge cases (no receipts, invalid ID, 'latest' resolution, error paths).
Alternative mitigations considered:
Rely on integration tests elsewhere, but unit tests are more targeted and maintainable.
Severity calibration:
Score 4 because the command is a user-facing feature critical to the receipt system; missing tests increase risk of regression.
Suggested fix shape:
func TestReceiptList_NoReceipts(t *testing.T) {
// mock receipt.List to return empty slice, capture output, assert message
}
func TestReceiptVerify_InvalidID(t *testing.T) {
// expect exit code 1 and error message
}
Why this wasn't caught:
The source file has no sibling test in this PR.
How can I resolve this? If you propose a fix, please make it concise.
| func TestNoEgressOutsideHttpx(t *testing.T) { | ||
| // Egress-creating symbols. Their only legitimate home is this package. | ||
| forbiddenSymbols := []string{ | ||
| "http.Client{", |
There was a problem hiding this comment.
Egress guard uses substring match against http.Client{ rather than structural import/construction check — false-positive/false-negative risk
The egress guard forbids the literal text http.Client{ in non-httpx .go files, but the codebase's own convention for creating clients is the httpx constructor httpx.NewClient(timeout). The guard's literal check will fire on every future conversion of a raw-client site to an httpx chokepoint that happens to have &http.Client{ in a commented-out line or in a doc-string example, and more importantly it does NOT check for the actual egress pattern this PR is fighting: constructing a raw http.Client with a timeout via a time.Duration literal (e.g. &http.Client{Timeout: 5 * time.Second}) in packages that do not import httpx. The guard tests the wrong shape — a textual substring instead of the semantics of the egress chokepoint — so it can both false-positive on harmless uses AND fail to detect a naive raw client that uses a Duration variable rather than the literal token.
Example:
// In tools.go or auth.go someone adds a raw client with constant-folding:
var timeout = 30 * time.Second
client := &http.Client{Timeout: timeout} // no literal "http.Client{" here — guard passes
The literal guard misses it because it only scans for the string "http.Client{" and doesn't enforce the semantic rule "every http.Client must come from httpx.NewClient". Concurrently, if someone writes a doc comment inside httpx.go containing "http.Client{" the guard still passes because httpx skips itself — but the two halves of the invariant are asymmetrical: the guard bans tokens, not the underlying importing/constructing relationship.
Suggested fix:
The guard should run on compilation artifacts, not source text. Instead of grepping for strings, the guard could examine each non-httpx package's compiled AST for imported net/http symbols and for composite literals of type `http.Client` or `http.Transport`; or it could rely on a simple BUILD rule that bans `"net/http"` import in all packages except httpx, combined with a `go vet` checker that flags raw `http.Client` construction. The current string-match approach is a useful first pass but won't survive the first refactor where someone uses a helper variable to set the timeout.More Info
- Threat model: A future refactor that constructs a raw client without the exact token
http.Client{(e.g. using a package-level variable or a helper) will slip past the guard, silently losing the egress receipt guarantee for that site. - Specific code citations:
internal/httpx/guard_test.go:52-54: theforbiddenSymbolsslice lists the literal strings it searches for.internal/agent/ollama.go:82:httpx.NewClient(120 * time.Second)— the real egress convention is the constructor, not the token. - Existing protections: The
httpxskips itself and test files are excluded from the walk, so the guard won't fire spuriously onhttpx.go's ownhttp.Client{Timeout:...}insideNewClient. But the guard doesn't verify that every non-httpx package importshttpxfor its HTTP needs — only that the banned strings don't appear. - Proposed mitigation: Replace the substring-grep with an AST or import-based check that verifies non-httpx packages neither import
net/httpnor constructhttp.Client/http.Transportcomposite literals. - Alternative mitigations considered: Replace the source-code grepping with an AST-based check (flag every
&http.Clientcomposite literal orhttp.NewRequestcall in non-httpx packages) or a simple banned-import rule ("net/http"forbidden outsidehttpx). Both approaches are structural and won't be defeated by minor refactoring. - Severity calibration: Score 3: the guard works against the exact patterns it was written for but the textual approach introduces a long-term maintenance risk — future false positives or false negatives as the codebase evolves.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/guard_test.go
Line: 24
Comment:
**Egress guard uses substring match against `http.Client{` rather than structural import/construction check — false-positive/false-negative risk**
The egress guard forbids the literal text `http.Client{` in non-httpx `.go` files, but the codebase's own convention for creating clients is the httpx constructor `httpx.NewClient(timeout)`. The guard's literal check will fire on every future conversion of a raw-client site to an httpx chokepoint that happens to have `&http.Client{` in a commented-out line or in a doc-string example, and more importantly it does NOT check for the actual egress pattern this PR is fighting: constructing a raw `http.Client` with a timeout via a `time.Duration` literal (e.g. `&http.Client{Timeout: 5 * time.Second}`) in packages that do not import httpx. The guard tests the wrong shape — a textual substring instead of the semantics of the egress chokepoint — so it can both false-positive on harmless uses AND fail to detect a naive raw client that uses a Duration variable rather than the literal token.
Example:
// In tools.go or auth.go someone adds a raw client with constant-folding:
var timeout = 30 * time.Second
client := &http.Client{Timeout: timeout} // no literal "http.Client{" here — guard passes
The literal guard misses it because it only scans for the string "http.Client{" and doesn't enforce the semantic rule "every http.Client must come from httpx.NewClient". Concurrently, if someone writes a doc comment inside httpx.go containing "http.Client{" the guard still passes because httpx skips itself — but the two halves of the invariant are asymmetrical: the guard bans tokens, not the underlying importing/constructing relationship.
Threat model:
A future refactor that constructs a raw client without the exact token `http.Client{` (e.g. using a package-level variable or a helper) will slip past the guard, silently losing the egress receipt guarantee for that site.
Specific code citations:
`internal/httpx/guard_test.go:52-54`: the `forbiddenSymbols` slice lists the literal strings it searches for. `internal/agent/ollama.go:82`: `httpx.NewClient(120 * time.Second)` — the real egress convention is the constructor, not the token.
Existing protections:
The `httpx` skips itself and test files are excluded from the walk, so the guard won't fire spuriously on `httpx.go`'s own `http.Client{Timeout:...}` inside `NewClient`. But the guard doesn't verify that every non-httpx package *imports* `httpx` for its HTTP needs — only that the banned strings don't appear.
Proposed mitigation:
Replace the substring-grep with an AST or import-based check that verifies non-httpx packages neither import `net/http` nor construct `http.Client` / `http.Transport` composite literals.
Alternative mitigations considered:
Replace the source-code grepping with an AST-based check (flag every `&http.Client` composite literal or `http.NewRequest` call in non-httpx packages) or a simple banned-import rule (`"net/http"` forbidden outside `httpx`). Both approaches are structural and won't be defeated by minor refactoring.
Severity calibration:
Score 3: the guard works against the exact patterns it was written for but the textual approach introduces a long-term maintenance risk — future false positives or false negatives as the codebase evolves.
Suggested fix shape:
The guard should run on compilation artifacts, not source text. Instead of grepping for strings, the guard could examine each non-httpx package's compiled AST for imported net/http symbols and for composite literals of type `http.Client` or `http.Transport`; or it could rely on a simple BUILD rule that bans `"net/http"` import in all packages except httpx, combined with a `go vet` checker that flags raw `http.Client` construction. The current string-match approach is a useful first pass but won't survive the first refactor where someone uses a helper variable to set the timeout.
How can I resolve this? If you propose a fix, please make it concise.
| // so identity/store stay separate from the qmax CLI's ~/.qamax layout. Both | ||
| // binaries emit the same receipt_version schema; only BaseDir differs. | ||
| func initReceiptPaths() { | ||
| if home, err := os.UserHomeDir(); err == nil { |
There was a problem hiding this comment.
initReceiptPaths silently skips setting BaseDir if $HOME is unavailable
Low-confidence finding — expand to read
If os.UserHomeDir() returns an error, receipt.BaseDir is never set, leaving it at the module default (possibly the current directory). The session receipt will then be written to an unexpected location, and no warning is printed. At minimum, log a warning when the home directory cannot be resolved, so the user knows where the receipt will land.
Prompt To Fix With AI
This is a comment left during a code review.
Path: receipt.go
Line: 17
Comment:
**initReceiptPaths silently skips setting BaseDir if $HOME is unavailable**
If `os.UserHomeDir()` returns an error, `receipt.BaseDir` is never set, leaving it at the module default (possibly the current directory). The session receipt will then be written to an unexpected location, and no warning is printed. At minimum, log a warning when the home directory cannot be resolved, so the user knows where the receipt will land.
How can I resolve this? If you propose a fix, please make it concise.
Address diff-risk-review findings on PR #149: B1 (blocker): signal-based exits (Ctrl+C x2, SIGTERM) called os.Exit(0) in repl.Run, bypassing main's deferred finalizeSessionReceipt — so the most common quit path silently dropped the receipt. Pass the finalizer into repl.Run as a sync.Once-wrapped callback; saveAndExit now calls it before os.Exit. Normal exits (defer) and signal exits (callback) both converge on the same once-guarded write. W1: VNC live-browser streaming dials a coder/websocket connection to the cloud sandbox, bypassing http.Transport and the receipt RoundTripper. Add coder/websocket to the egress guard's forbidden-import list and allowlist the vnc package as a documented carve-out (carries pixels, never source/prompts). Any new WebSocket egress must be reviewed. W2: Document the body-hash assumption — ReqBytes/ReqSHA256 are read after RoundTrip returns, which is accurate for all current endpoints (they drain the request body before responding); noted for future early-responding servers.
There was a problem hiding this comment.
1 finding outside the diff
| File | Scope | Finding |
|---|---|---|
| internal/httpx/guard_test.go:86 | file-scope | Guard test flags forbidden imports inside comments, causing false positives |
| return nil | ||
| } | ||
| if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { | ||
| return nil |
There was a problem hiding this comment.
Guard test does not skip testdata directories, causing false positives on fixture files
The walk skips vendor, build, and hidden directories but does not exclude testdata directories. Go testdata files are fixture data, not production code, and may legitimately contain http.NewRequest or third-party imports for test setup. The guard will flag them and break the build, forcing developers to either refactor test fixtures or carve out testdata directories.
More Info
- Threat model: A test fixture file under
testdata/useshttp.NewRequest(for example, to generate recorded responses). The guard scan, which runs over all.gofiles, will treat it as an egress violation and fail CI. - Specific code citations: Skip condition at line 63:
if name == "httpx" || name == "vnc" || name == "vendor" || ...— notestdatacase. - Existing protections: Only
_test.gofiles are excluded;testdatafiles are not test files and are scanned. - Proposed mitigation: Add
name == "testdata"to the directory skip list sofilepath.SkipDiris returned for testdata directories. - Severity calibration: Moderate: breaks CI for a legitimate pattern that is explicitly not egress. Blast radius is limited to projects with test fixtures using HTTP. Score 3.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/guard_test.go
Line: 63
Comment:
**Guard test does not skip testdata directories, causing false positives on fixture files**
The walk skips `vendor`, `build`, and hidden directories but does not exclude `testdata` directories. Go testdata files are fixture data, not production code, and may legitimately contain `http.NewRequest` or third-party imports for test setup. The guard will flag them and break the build, forcing developers to either refactor test fixtures or carve out testdata directories.
Threat model:
A test fixture file under `testdata/` uses `http.NewRequest` (for example, to generate recorded responses). The guard scan, which runs over all `.go` files, will treat it as an egress violation and fail CI.
Specific code citations:
Skip condition at line 63: `if name == "httpx" || name == "vnc" || name == "vendor" || ...` — no `testdata` case.
Existing protections:
Only `_test.go` files are excluded; `testdata` files are not test files and are scanned.
Proposed mitigation:
Add `name == "testdata"` to the directory skip list so `filepath.SkipDir` is returned for testdata directories.
Severity calibration:
Moderate: breaks CI for a legitimate pattern that is explicitly not egress. Blast radius is limited to projects with test fixtures using HTTP. Score 3.
How can I resolve this? If you propose a fix, please make it concise.
| // Ignore comments to avoid false positives in prose. | ||
| code := line | ||
| if idx := strings.Index(code, "//"); idx >= 0 { | ||
| code = code[:idx] |
There was a problem hiding this comment.
Static guard's comment stripping may miss multi-line comments, causing false positives
The guard test strips inline comments by taking code = code[:idx] where idx is the first // occurrence. This does not handle block comments (/* ... */) or // inside string literals, which could cause false-positive violations if a forbidden symbol appears inside a comment or string. The test could fail the build incorrectly, breaking CI for legitimate code.
More Info
- Threat model: Developers adding legitimate comments or documentation that contain the forbidden symbol strings (e.g.,
http.Client{in a code example) would cause the guard test to fail CI, blocking merges unnecessarily. - Specific code citations: Lines 66-73:
if idx := strings.Index(code, "//"); idx >= 0 { code = code[:idx] } - Existing protections: None; the guard currently assumes
//only appears as an inline comment delimiter. - Proposed mitigation: Use
go/parserto parse each file and inspect only the actual AST nodes, skipping comments and string literals. Alternatively, implement a simple scanner that tracks block comment state. - Alternative mitigations considered: Keep the simple string search but add a pre‑filter that skips lines where the symbol appears inside a string literal (between quotes) or after a
//that is not the first//on the line. This is error‑prone but better than nothing. - Severity calibration: Score 3 because a false positive would break CI and block merges, but the impact is limited to development friction, not a runtime bug. The fix is straightforward and the risk is moderate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/guard_test.go
Line: 74
Comment:
**Static guard's comment stripping may miss multi-line comments, causing false positives**
The guard test strips inline comments by taking `code = code[:idx]` where `idx` is the first `//` occurrence. This does not handle block comments (`/* ... */`) or `//` inside string literals, which could cause false-positive violations if a forbidden symbol appears inside a comment or string. The test could fail the build incorrectly, breaking CI for legitimate code.
Threat model:
Developers adding legitimate comments or documentation that contain the forbidden symbol strings (e.g., `http.Client{` in a code example) would cause the guard test to fail CI, blocking merges unnecessarily.
Specific code citations:
Lines 66-73: `if idx := strings.Index(code, "//"); idx >= 0 { code = code[:idx] }`
Existing protections:
None; the guard currently assumes `//` only appears as an inline comment delimiter.
Proposed mitigation:
Use `go/parser` to parse each file and inspect only the actual AST nodes, skipping comments and string literals. Alternatively, implement a simple scanner that tracks block comment state.
Alternative mitigations considered:
Keep the simple string search but add a pre‑filter that skips lines where the symbol appears inside a string literal (between quotes) or after a `//` that is not the first `//` on the line. This is error‑prone but better than nothing.
Severity calibration:
Score 3 because a false positive would break CI and block merges, but the impact is limited to development friction, not a runtime bug. The fix is straightforward and the risk is moderate.
How can I resolve this? If you propose a fix, please make it concise.
| @@ -0,0 +1,98 @@ | |||
| package httpx | |||
There was a problem hiding this comment.
Guard test lacks verification of its own effectiveness
The static egress guard is a critical build-time enforcement, but there is no test that validates the guard actually catches violations. A unit test could inject a synthetic Go file containing a forbidden symbol and assert the guard fails, and another with allowed symbols (e.g., http.Request type usage) and assert it passes. Without such a test, we cannot be confident the guard works as intended.
Example:
If the guard's pattern matching is buggy (e.g., misses `http.NewRequest(` with a space after the parenthesis), a violation could slip through.
Suggested fix:
Add a subtest that writes a temporary .go file with a violation and expects t.Fatalf, and another with only allowed http usage and expects no failure.Why this wasn't caught: The guard test is itself untested; no test ensures it will fail when a real violation is introduced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/guard_test.go
Line: 1
Comment:
**Guard test lacks verification of its own effectiveness**
The static egress guard is a critical build-time enforcement, but there is no test that validates the guard actually catches violations. A unit test could inject a synthetic Go file containing a forbidden symbol and assert the guard fails, and another with allowed symbols (e.g., `http.Request` type usage) and assert it passes. Without such a test, we cannot be confident the guard works as intended.
Example:
If the guard's pattern matching is buggy (e.g., misses `http.NewRequest(` with a space after the parenthesis), a violation could slip through.
Suggested fix shape:
Add a subtest that writes a temporary .go file with a violation and expects t.Fatalf, and another with only allowed http usage and expects no failure.
Why this wasn't caught:
The guard test is itself untested; no test ensures it will fail when a real violation is introduced.
How can I resolve this? If you propose a fix, please make it concise.
…ction test
- receiptShow: check json.MarshalIndent error instead of discarding it;
print diagnostic to stderr and exit non-zero on failure (Sigilix P2).
- Refactor egress guard: extract scanForEgressViolations into a testable
function and add TestEgressGuardDetectsInjection, which writes a raw
http.Client{} to a temp dir and asserts the scan catches it — making
the 'verified by injection' acceptance criterion permanent in CI
(Sigilix P3).
Contributor
Author
Sigilix review — resolutionFixed in
Dismissed — false positive:
Deferred (out of scope for this PR):
|
There was a problem hiding this comment.
1 finding outside the diff
| File | Scope | Finding |
|---|---|---|
| cmd_receipt.go:100 | file-scope | Path traversal via user-controlled receipt ID in mustResolveReceipt |
Comment on lines
+85
to
+98
| func mustResolveReceipt(idOrLatest string) *receipt.Receipt { | ||
| dir, err := receipt.ReceiptsDir() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| if idOrLatest == "" || idOrLatest == "latest" { | ||
| paths, err := receipt.List() | ||
| if err != nil || len(paths) == 0 { | ||
| fmt.Fprintln(os.Stderr, "No exposure receipts found.") | ||
| os.Exit(1) | ||
| } | ||
| r, err := receipt.Load(paths[len(paths)-1]) | ||
| if err != nil { |
There was a problem hiding this comment.
Unvalidated user input used as path component allows reading arbitrary files outside the receipts directory
Flagged by 2 specialists.
Example:
input: qmax-code receipt show ../../../etc/passwd
actual: filepath.Join("/home/user/.qmax-code/receipts", "../../../etc/passwd.json") resolves to "/etc/passwd.json"; receipt.Load attempts to parse it as JSON, producing a confusing error or partial display
expected: tool rejects the ID with "invalid receipt ID: ../../../etc/passwd"
Suggested fix:
if !receiptIDPattern.MatchString(idOrLatest) {
fmt.Fprintf(os.Stderr, "invalid receipt ID: %s\n", idOrLatest)
os.Exit(1)
}
r, err := receipt.Load(filepath.Join(dir, idOrLatest+".json"))Why this wasn't caught: No test exercises mustResolveReceipt with path-traversal inputs.
Detailed reasoning
mustResolveReceipt takes user-controlled idOrLatest and constructs a file path with filepath.Join(dir, idOrLatest+".json"). filepath.Join calls filepath.Clean, which resolves .. components, so an input like ../../../etc/passwd escapes the receipts directory and reads an arbitrary .json file on disk.
While this is a local CLI tool running with the user's own permissions (so there's no privilege escalation), the tool is positioned as security-aware ('Receipts, not promises'). The function should validate that idOrLatest matches the expected receipt-ID format (e.g. UUID pattern) before using it as a path component, and reject unrecognized formats with a clear error instead of silently treating them as file paths.
More Info
- Threat model: A user passing a crafted ID like
../../.ssh/configcould cause the tool to attempt loading a non-receipt file as a receipt, producing confusing parse errors or, in edge cases, leaking file metadata through JSON error messages. The user already has OS-level read access, so the harm is limited to unexpected tool behavior rather than privilege escalation. - Specific code citations:
cmd_receipt.goline 98:filepath.Join(dir, idOrLatest+".json")— user input inserted directly into the path with no validation.filepath.Joindocumentation confirms it callsClean, which resolves..components. - Existing protections: None —
idOrLatestis checked only for""and"latest"(which are handled separately withreceipt.List()). All other values are passed directly tofilepath.Join. - Proposed mitigation: Validate
idOrLatestagainst a receipt-ID format (e.g.regexp.MustCompile(\^[a-f0-9-]+$`)for UUID v4/v7). Reject non-matching inputs withfmt.Fprintf(os.Stderr, "invalid receipt ID: %s\n", idOrLatest)andos.Exit(1)`. This closes the path traversal while also giving clearer feedback for typos. - Alternative mitigations considered:
filepath.Base(idOrLatest)strips directory components but still allows reading any.jsonfile within the receipts directory — better than nothing but doesn't validate the format. A UUID regex is stronger because it also catches typos likelatestt. - Severity calibration: Score 4: Exploitation requires the attacker to control the command argument, which is plausible in scripted or automated environments, and the impact is arbitrary file read. Although the tool runs with user permissions (no privilege escalation), the path traversal is a real logic gap in a security-positioned tool, and the fix is cheap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd_receipt.go
Line: 85-98
Comment:
**Unvalidated user input used as path component allows reading arbitrary files outside the receipts directory**
_Flagged by 2 specialists._
`mustResolveReceipt` takes user-controlled `idOrLatest` and constructs a file path with `filepath.Join(dir, idOrLatest+".json")`. `filepath.Join` calls `filepath.Clean`, which resolves `..` components, so an input like `../../../etc/passwd` escapes the receipts directory and reads an arbitrary `.json` file on disk.
While this is a local CLI tool running with the user's own permissions (so there's no privilege escalation), the tool is positioned as security-aware ('Receipts, not promises'). The function should validate that `idOrLatest` matches the expected receipt-ID format (e.g. UUID pattern) before using it as a path component, and reject unrecognized formats with a clear error instead of silently treating them as file paths.
Example:
input: qmax-code receipt show ../../../etc/passwd
actual: filepath.Join("/home/user/.qmax-code/receipts", "../../../etc/passwd.json") resolves to "/etc/passwd.json"; receipt.Load attempts to parse it as JSON, producing a confusing error or partial display
expected: tool rejects the ID with "invalid receipt ID: ../../../etc/passwd"
Threat model:
A user passing a crafted ID like `../../.ssh/config` could cause the tool to attempt loading a non-receipt file as a receipt, producing confusing parse errors or, in edge cases, leaking file metadata through JSON error messages. The user already has OS-level read access, so the harm is limited to unexpected tool behavior rather than privilege escalation.
Specific code citations:
`cmd_receipt.go` line 98: `filepath.Join(dir, idOrLatest+".json")` — user input inserted directly into the path with no validation. `filepath.Join` documentation confirms it calls `Clean`, which resolves `..` components.
Existing protections:
None — `idOrLatest` is checked only for `""` and `"latest"` (which are handled separately with `receipt.List()`). All other values are passed directly to `filepath.Join`.
Proposed mitigation:
Validate `idOrLatest` against a receipt-ID format (e.g. `regexp.MustCompile(\`^[a-f0-9-]+$\`)` for UUID v4/v7). Reject non-matching inputs with `fmt.Fprintf(os.Stderr, "invalid receipt ID: %s\n", idOrLatest)` and `os.Exit(1)`. This closes the path traversal while also giving clearer feedback for typos.
Alternative mitigations considered:
`filepath.Base(idOrLatest)` strips directory components but still allows reading any `.json` file within the receipts directory — better than nothing but doesn't validate the format. A UUID regex is stronger because it also catches typos like `latestt`.
Severity calibration:
Score 4: Exploitation requires the attacker to control the command argument, which is plausible in scripted or automated environments, and the impact is arbitrary file read. Although the tool runs with user permissions (no privilege escalation), the path traversal is a real logic gap in a security-positioned tool, and the fix is cheap.
Suggested fix shape:
if !receiptIDPattern.MatchString(idOrLatest) {
fmt.Fprintf(os.Stderr, "invalid receipt ID: %s\n", idOrLatest)
os.Exit(1)
}
r, err := receipt.Load(filepath.Join(dir, idOrLatest+".json"))
Why this wasn't caught:
No test exercises `mustResolveReceipt` with path-traversal inputs.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+85
to
+92
| func mustResolveReceipt(idOrLatest string) *receipt.Receipt { | ||
| dir, err := receipt.ReceiptsDir() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| if idOrLatest == "" || idOrLatest == "latest" { | ||
| paths, err := receipt.List() |
There was a problem hiding this comment.
receipt.List() error is silently discarded, hiding the real failure from the user
In mustResolveReceipt, when receipt.List() returns an error, the code prints a generic "No exposure receipts found." message and exits. The actual error (permission denied, directory missing, I/O error) is never shown. The user cannot distinguish between 'you have no receipts' and 'I cannot read your receipts directory due to a filesystem error,' which makes troubleshooting impossible and could hide a misconfiguration (e.g. wrong HOME or permission issue).
More Info
- Threat model: A user who has a legitimate receipts directory but hits a transient permission or filesystem error sees the same message as a user with genuinely zero receipts. They may conclude the tool isn't working, blame the feature, or miss a misconfiguration that prevents receipt collection entirely.
- Specific code citations:
cmd_receipt.golines 88-92:paths, err := receipt.List(); if err != nil || len(paths) == 0 { fmt.Fprintln(os.Stderr, "No exposure receipts found."); os.Exit(1) }— theerrvariable is checked but never printed. - Existing protections: The
receiptList()function separately handles the same condition correctly (prints the error before exiting), showing the pattern the codebase intends to use.mustResolveReceiptdiverges from this pattern. - Proposed mitigation: Split the condition: check
err != nilfirst, print the error, and exit with code 1. Then checklen(paths) == 0and print the 'No receipts' message. This matches the pattern already used inreceiptList(). - Alternative mitigations considered: A single
if err != nil || len(paths) == 0withfmt.Fprintf(os.Stderr, "Error listing receipts: %v\n", err)would print<nil>in the 'no receipts' case, which is confusing. Splitting is cleaner. - Severity calibration: Score 3: Not a correctness bug (the program still exits), but a user-facing robustness gap that violates the principle of least surprise and makes real-world troubleshooting harder. The fix is a simple two-line restructure mirroring the already-correct
receiptList()in the same file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd_receipt.go
Line: 85-92
Comment:
**receipt.List() error is silently discarded, hiding the real failure from the user**
In `mustResolveReceipt`, when `receipt.List()` returns an error, the code prints a generic "No exposure receipts found." message and exits. The actual error (permission denied, directory missing, I/O error) is never shown. The user cannot distinguish between 'you have no receipts' and 'I cannot read your receipts directory due to a filesystem error,' which makes troubleshooting impossible and could hide a misconfiguration (e.g. wrong `HOME` or permission issue).
Threat model:
A user who has a legitimate receipts directory but hits a transient permission or filesystem error sees the same message as a user with genuinely zero receipts. They may conclude the tool isn't working, blame the feature, or miss a misconfiguration that prevents receipt collection entirely.
Specific code citations:
`cmd_receipt.go` lines 88-92: `paths, err := receipt.List(); if err != nil || len(paths) == 0 { fmt.Fprintln(os.Stderr, "No exposure receipts found."); os.Exit(1) }` — the `err` variable is checked but never printed.
Existing protections:
The `receiptList()` function separately handles the same condition correctly (prints the error before exiting), showing the pattern the codebase intends to use. `mustResolveReceipt` diverges from this pattern.
Proposed mitigation:
Split the condition: check `err != nil` first, print the error, and exit with code 1. Then check `len(paths) == 0` and print the 'No receipts' message. This matches the pattern already used in `receiptList()`.
Alternative mitigations considered:
A single `if err != nil || len(paths) == 0` with `fmt.Fprintf(os.Stderr, "Error listing receipts: %v\n", err)` would print `<nil>` in the 'no receipts' case, which is confusing. Splitting is cleaner.
Severity calibration:
Score 3: Not a correctness bug (the program still exits), but a user-facing robustness gap that violates the principle of least surprise and makes real-world troubleshooting harder. The fix is a simple two-line restructure mirroring the already-correct `receiptList()` in the same file.
How can I resolve this? If you propose a fix, please make it concise.
| case "verify": | ||
| receiptVerify(argAt(args, 1)) | ||
| default: | ||
| fmt.Fprintln(os.Stderr, "Usage: qmax-code receipt <list|show|verify> [id|latest]") |
There was a problem hiding this comment.
New CLI command qmax-code receipt ships without unit tests for error paths and edge cases.
The new cmd_receipt.go file implements a customer-facing CLI command with three subcommands (list, show, verify) and handles several error conditions (missing receipts, invalid IDs, load failures). The [TESTING EVIDENCE] block shows siblingMapping:cmd_receipt.go->NONE — no sibling test file was added in this PR. Without unit tests, regressions in argument parsing, error handling, and the latest resolution logic could silently break the user experience.
Add a test file cmd_receipt_test.go that exercises the subcommand dispatch, validates argAt behavior, and asserts correct exit codes and output for error cases like missing receipts, invalid IDs, and load failures.
Example:
Input: `qmax-code receipt verify non-existent-id`
Expected: Prints error message and exits with code 1.
Actual: Could panic or exit with code 0 if error handling regresses.
Suggested fix:
func TestReceiptVerifyMissingId(t *testing.T) {
// Mock receipt.Load to return error
// Call handleReceiptCommand with args ["verify", "non-existent"]
// Assert stderr contains error and os.Exit(1) is called (use os.Exit override)
}Why this wasn't caught: The sibling mapping shows no test file for cmd_receipt.go in this PR.
More Info
- Threat model: A user running
qmax-code receipt verify latestcould receive a misleading success message or crash if the receipt loading logic regresses, undermining trust in the exposure receipt guarantee. - Specific code citations:
cmd_receipt.golines 13-30 define subcommand dispatch; lines 32-38 implementargAt; lines 40-110 implementreceiptList,receiptShow,receiptVerify, andmustResolveReceiptwith multiple error exits. - Existing protections: No unit tests exist for this file per the sibling mapping. The CI runs
go testbut will not catch regressions in this command's behavior. - Proposed mitigation: Create
cmd_receipt_test.gowith table-driven tests for each subcommand, covering success cases, missing receipts, invalid IDs, and load failures. Mock thereceiptpackage functions where appropriate to isolate CLI logic. - Alternative mitigations considered: Relying on integration tests elsewhere is insufficient because they may not exercise all error paths of this specific command.
- Severity calibration: Score 4 because the CLI is a user-facing feature and untested error handling could lead to confusing or broken user experience, though it does not directly affect the security guarantee of the receipts themselves.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd_receipt.go
Line: 31
Comment:
**New CLI command `qmax-code receipt` ships without unit tests for error paths and edge cases.**
The new `cmd_receipt.go` file implements a customer-facing CLI command with three subcommands (`list`, `show`, `verify`) and handles several error conditions (missing receipts, invalid IDs, load failures). The [TESTING EVIDENCE] block shows `siblingMapping:cmd_receipt.go->NONE` — no sibling test file was added in this PR. Without unit tests, regressions in argument parsing, error handling, and the `latest` resolution logic could silently break the user experience.
Add a test file `cmd_receipt_test.go` that exercises the subcommand dispatch, validates `argAt` behavior, and asserts correct exit codes and output for error cases like missing receipts, invalid IDs, and load failures.
Example:
Input: `qmax-code receipt verify non-existent-id`
Expected: Prints error message and exits with code 1.
Actual: Could panic or exit with code 0 if error handling regresses.
Threat model:
A user running `qmax-code receipt verify latest` could receive a misleading success message or crash if the receipt loading logic regresses, undermining trust in the exposure receipt guarantee.
Specific code citations:
`cmd_receipt.go` lines 13-30 define subcommand dispatch; lines 32-38 implement `argAt`; lines 40-110 implement `receiptList`, `receiptShow`, `receiptVerify`, and `mustResolveReceipt` with multiple error exits.
Existing protections:
No unit tests exist for this file per the sibling mapping. The CI runs `go test` but will not catch regressions in this command's behavior.
Proposed mitigation:
Create `cmd_receipt_test.go` with table-driven tests for each subcommand, covering success cases, missing receipts, invalid IDs, and load failures. Mock the `receipt` package functions where appropriate to isolate CLI logic.
Alternative mitigations considered:
Relying on integration tests elsewhere is insufficient because they may not exercise all error paths of this specific command.
Severity calibration:
Score 4 because the CLI is a user-facing feature and untested error handling could lead to confusing or broken user experience, though it does not directly affect the security guarantee of the receipts themselves.
Suggested fix shape:
func TestReceiptVerifyMissingId(t *testing.T) {
// Mock receipt.Load to return error
// Call handleReceiptCommand with args ["verify", "non-existent"]
// Assert stderr contains error and os.Exit(1) is called (use os.Exit override)
}
Why this wasn't caught:
The sibling mapping shows no test file for cmd_receipt.go in this PR.
How can I resolve this? If you propose a fix, please make it concise.
| // TestEgressGuardDetectsInjection proves the guard actually catches a raw | ||
| // http.Client construction and a forbidden import — so the guard's detection | ||
| // logic itself is validated in CI, not just the absence of violations today. | ||
| func TestEgressGuardDetectsInjection(t *testing.T) { |
There was a problem hiding this comment.
Guard injection test only checks for http.Client{} symbol, missing other forbidden constructs.
TestEgressGuardDetectsInjection writes a test file containing only var client = &http.Client{} and asserts the guard flags it. This validates detection of that specific symbol but does not prove the guard catches all forbidden symbols (e.g., http.NewRequest, http.Get) or forbidden imports. A regression that breaks detection of other symbols could go unnoticed.
Expand the injection test to include at least one example of each forbidden symbol category (client construction, request creation, default client, third-party import) to ensure the guard's scanning logic is comprehensive.
Example:
If the guard's scanning logic has a bug that only affects `http.NewRequest` detection, the injection test would still pass because it only injects `http.Client{}`.
Suggested fix:
Add additional test files in the temp dir:
- `bad2.go` with `req, _ := http.NewRequest(...)`
- `bad3.go` with `import "github.com/go-resty/resty/v2"`
Then assert violations contain entries for each.Why this wasn't caught: The existing test only validates detection of one forbidden symbol.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/httpx/guard_test.go
Line: 116
Comment:
**Guard injection test only checks for `http.Client{}` symbol, missing other forbidden constructs.**
`TestEgressGuardDetectsInjection` writes a test file containing only `var client = &http.Client{}` and asserts the guard flags it. This validates detection of that specific symbol but does not prove the guard catches all forbidden symbols (e.g., `http.NewRequest`, `http.Get`) or forbidden imports. A regression that breaks detection of other symbols could go unnoticed.
Expand the injection test to include at least one example of each forbidden symbol category (client construction, request creation, default client, third-party import) to ensure the guard's scanning logic is comprehensive.
Example:
If the guard's scanning logic has a bug that only affects `http.NewRequest` detection, the injection test would still pass because it only injects `http.Client{}`.
Suggested fix shape:
Add additional test files in the temp dir:
- `bad2.go` with `req, _ := http.NewRequest(...)`
- `bad3.go` with `import "github.com/go-resty/resty/v2"`
Then assert violations contain entries for each.
Why this wasn't caught:
The existing test only validates detection of one forbidden symbol.
How can I resolve this? If you propose a fix, please make it concise.
Desperado
deleted the
strazhnyk/qua-1316-step-3-exposure-receipt-in-qmax-code
branch
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.
Closes QUA-1316 · Parent: QUA-1315
What
Brings the Exposure Receipt to the
qmax-codeterminal agent so both customer-facing binaries emit receipts — making "Receipts, not promises" a platform property, not a CLI-only one.qmax-codehas its own egress (LLM inference +internal/apicloud calls) that theqmaxCLI's receipt does not cover; they are peers, not one wrapping the other.Changes
internal/httpx— single egress chokepoint. RecordingRoundTripperstream-hashes the request body (no buffering), records destination/category/size/SHA-256 — never content.NewClient+NewRequestreplace all rawhttp.Client/http.NewRequestacross all ~13 egress sites (api client, auth, codex/cc connect, anthropic + cerebras + ollama LLM calls, script fetch/update, vision, login, ollama probe).WithModelattributes the model on LLM entries.internal/httpx/guard_test.go— static egress guard fails the build if any package outside httpx constructs a raw http client/request or imports a third-party HTTP lib. Wired intoci.ymlvia a newgo teststep (CI ran no tests before) — un-receipted egress can't merge. Verified by injection.internal/exposure— qmax-code's own taxonomy (llm-prompt,llm-completion,cloud-api,mcp-traffic,control,uncategorized) +Classify.github.com/Quality-Max/qmax-receiptv0.1.0 — one versioned contract, onereceipt_version, no local copy.NewCurrentat startup, finalized at exit (only if the session egressed).BaseDir = ~/.qmax-codekeeps identity/store separate from the CLI's~/.qmax.qmax-code receipt <list|show|verify>— offline verify prints the provenance-not-honesty caveat.release.ymlstampsagent_build_shavia-X qmax-receipt.BuildSHA.Acceptance criteria
httpx; static guard fails the build on any rawhttp.Clientoutside it, verified by injecting one.~/.qmax-code/receipts/<id>.json;qmax-code receipt verifyvalidates it offline (TestFinalizeAndVerifyRoundTrip).TestRecordsRequestHashAndSize).receipt_versionschema, and onereceipt verifyvalidates a receipt from either.go build/vet/go testgreen; guard runs inci.yml.Test plan
go build ./.../go vet ./...passgo test ./internal/httpx/... ./internal/exposure/...pass (guard, hashing, verify round-trip, taxonomy)http.Clientoutside httpx → guard fails → revertKnown follow-up
RespBytesis-1for SSE/chunked streaming LLM responses (inbound, not egress — doesn't weaken the guarantee); response-side streaming hash tracked separately.