Address Copilot review feedback from PR #16 - #24
Open
ndenny wants to merge 1 commit into
Open
Conversation
Docs: - README/skills no longer claim create commands read JSON from stdin *only* — cli.GetBody also accepts CLI Shorthand args. Heredoc stdin is still the recommended form, and the "no --body/--data/-d" fact stands. - README: demote Manual installation / MacOS / Windows / Linux to subheadings under Installation so the hierarchy reads correctly. - setup-api-monitor.md: the scheduled target is the call ID, not the "monitor ID", matching the rest of that skill. Code: - skills: updateClaudeMD no longer swallows os.ReadFile errors. An unreadable-but-present CLAUDE.md was treated as empty and then overwritten, destroying its contents. Tests: - oauth: cover the reflected-XSS hardening in the auth callback handler. Both new escaping tests fail if html.EscapeString is removed. - skills: table-driven tests for resolveTarget (no flag, each flag, and every mutually-exclusive combination). - cli: TestAuthHookFailure was order-dependent — it needs a non-nil configs map and rsh-profile set, or it panics before reaching the auth hook. It now runs standalone. - cli: typo in TestLoadCache comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses prior automated review feedback from PR #16 by hardening file-update error handling, adding targeted test coverage for OAuth callback XSS escaping and skills target resolution, stabilizing a CLI request test that depended on global state, and correcting documentation to reflect actual input handling.
Changes:
- Prevent
CLAUDE.mdoverwrites whenos.ReadFilefails for reasons other than non-existence. - Add/extend unit tests for
resolveTargetand the OAuth auth callback handler (escaping + code passthrough). - Update docs (README + embedded skill docs) to reflect “stdin or CLI shorthand” request bodies and fix heading/wording issues.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/skills.go | Returns non-IsNotExist read errors from updateClaudeMD; updates generated agent guide wording about input sources. |
| skills/skills_test.go | Adds table-driven tests covering resolveTarget flag combinations and errors. |
| skills/embed/setup-mcp-monitor.md | Updates skill doc to reflect stdin or CLI shorthand input (preferring heredoc stdin). |
| skills/embed/setup-browser-monitor.md | Same stdin/shorthand doc correction for browser monitor skill. |
| skills/embed/setup-api-monitor.md | Same stdin/shorthand doc correction; fixes validation gate wording (“call ID”). |
| README.md | Fixes heading levels and clarifies stdin vs CLI shorthand for create/update bodies. |
| oauth/authcode_test.go | Adds tests validating auth callback HTML escapes attacker-controlled query params and still passes the code through. |
| cli/request_test.go | Stabilizes TestAuthHookFailure by initializing configs and setting rsh-profile. |
| cli/cli_test.go | Fixes a spelling typo in a comment. |
Suppressed comments (1)
oauth/authcode_test.go:71
- Close the response body returned by httptest's recorder here as well; keeping the close pattern consistent across the file helps prevent future leaks when this helper grows.
body, err := io.ReadAll(w.Result().Body)
assert.NoError(t, err)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+74
to
+81
| // This test must not depend on what earlier tests left behind: `configs` is | ||
| // nil until initConfig runs, and an empty rsh-profile makes MakeRequest | ||
| // panic with "invalid profile" before it ever reaches the auth hook. | ||
| if configs == nil { | ||
| configs = apiConfigs{} | ||
| } | ||
| viper.Set("rsh-profile", "default") | ||
|
|
Comment on lines
+35
to
+36
| body, err := io.ReadAll(w.Result().Body) | ||
| assert.NoError(t, err) |
Comment on lines
+72
to
+76
| if c.wantErr != "" { | ||
| assert.EqualError(t, err, c.wantErr) | ||
| assert.Empty(t, target) | ||
| return | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the Copilot review comments on #16 (the v0.0.1 release candidate) so that PR can go into main clean.
Fixed
skills/skills.go:176updateClaudeMDignoredos.ReadFileerrors — an existing-but-unreadableCLAUDE.mdwas treated as empty and then overwritten, destroying its contentsos.IsNotExistis treated as empty; any other read error is returnedoauth/authcode.go:240html.EscapeStringis removedskills/skills.go:78resolveTargethad no testscli/request_test.go:75TestAuthHookFailuredepended on global state left by earlier testsrsh-profiletodefault. It also needed a non-nilconfigsmap — Copilot spotted one of the two causes; both are fixed, and the test now passes standaloneREADME.md×2,skills/skills.go:208, 3×skills/embed/*.mdcli.GetBody(and the README's own CLI Shorthand section immediately below)--body/--data/-dflag" statement is keptREADME.md:12InstallationandManual installationwere both##Manual installation,MacOS,WindowsandLinuxdemoted to###underInstallationskills/embed/setup-api-monitor.md:92<call-id><monitor-id>and are unchangedNot fixed — the premise is incorrect
main.go:13— Copilot claimed the goreleaser-X main.commit/main.date/main.builtByldflags "will cause release builds to fail with 'cannot set -X main.commit' errors".The Go linker silently ignores
-Xfor symbols that don't exist; it is not an error. Verified two ways:go build -ldflags "-X main.commit=abc123 -X main.nonexistent=foo" .exits 0.v0.1.0-beta-1.The flags are dead config rather than a build break. Left alone here — rewriting five
.goreleaser/*.yamlfiles to strip no-ops immediately before the first formal release is risk without benefit. Worth a follow-up to either drop them or wirecommit/dateinto--version, whichversionExtraInfoalready exists to serve.Verification
go build ./...,go vet ./...andgo test ./...all pass. (gofmt -lflags five files —bulk/commands.go,bulk/metadata.go,cli/api.go,cli/autoconfig.go,cli/param.go— all pre-existing and untouched here.)🤖 Generated with Claude Code