Skip to content

chore(#7019): drop the last "interim" and rewrap two comments - #7032

Open
waynesun09 wants to merge 1 commit into
mainfrom
fullsend-7019-interim-followup
Open

chore(#7019): drop the last "interim" and rewrap two comments#7032
waynesun09 wants to merge 1 commit into
mainfrom
fullsend-7019-interim-followup

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

The two review-agent findings on #7025 that were still open when it reached the merge queue. Both are comments only — no behaviour change, 3 lines.

internal/runtime/pi_run.go — the -e block still called it "The interim Claude-on-Vertex provider". Dropping "interim" from the extension's characterization is what #7019 was for; that sweep covered pi.go, the Containerfile and runtime-implementation.md but missed this one. Now "The vendored Claude-on-Vertex extension".

internal/runtime/pi_extension/fullsend-agent.js — the childEnv comment was left at 98 columns by #7025's rewrite of its stray-credential rationale, in a file that wraps at 79. Rewrapped at the sentence boundary; wording unchanged.

Why these came separately

They were found by fullsend-ai-review[bot] on an earlier head of #7025 and confirmed still open against the final one. By then #7025 was enqueued and its branch was locked (protected branch hook declined), and it had behaviour + e2e green — so pushing a two-comment fix would have thrown away a full CI cycle for cosmetics. The change request was dismissed as "cosmetic, carried in a follow-up" and both threads resolved with that pointer; this is that follow-up.

The same review's other two findings needed no change: [protected-path] on images/ is informational and satisfied by the human approval on #7025, and [comment-formatting] on pi_run.go:25 was superseded when that comment was rewritten for the pi 0.85.0 bump.

Testing

$ go build ./... && go vet ./...
$ go test -count=1 ./internal/runtime/
ok  github.com/fullsend-ai/fullsend/internal/runtime  27.861s

$ node --test internal/runtime/pi_extension/fullsend-agent.test.mjs
# pass 32, fail 0

$ grep -c interim internal/runtime/pi_run.go
0
$ awk 'length>79 && /^[[:space:]]*\/\//' internal/runtime/pi_extension/fullsend-agent.js
# (no output)

Follow-up to #7025.

Addresses the review-agent findings on bedf185 that were still open.

pi_run.go's `-e` block still called it "the interim Claude-on-Vertex
provider". Removing that word from the extension's characterization is what
#7019 is for, and the sweep covered pi.go, the Containerfile and the docs
but missed this one. It is now "the vendored Claude-on-Vertex extension".

fullsend-agent.js's childEnv comment was left at 98 columns by the earlier
rewrite of its stray-credential rationale, in a file that wraps at 79.
Rewrapped at the sentence boundary; no wording change.

The review's third finding, [protected-path] on images/README.md and
images/sandbox/Containerfile, is informational -- it asks for human
approval of protected-path changes, which the PR has. Its second
[comment-formatting] finding, pi_run.go:25 at 104 columns, no longer
applies: that comment was rewritten in 4cc8e34 for the 0.85.0 bump and
now wraps within the file's convention.

Assisted-by: Claude (fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Clarify vendored Vertex extension comments and restore line wrapping

📝 Documentation 🕐 Less than 5 minutes

Grey Divider

AI Description

• Replaces outdated “interim” terminology with the extension’s current vendored status.
• Rewraps the child environment rationale to match the JavaScript file’s 79-column convention.
High-Level Assessment

The targeted comment edits are optimal for this cosmetic follow-up. Broader documentation changes or automated formatting would add unrelated scope without improving behavior or clarity.

Files changed (2) +3 / -3

Documentation (2) +3 / -3
fullsend-agent.jsRewrap child environment credential-hygiene comment +2/-2

Rewrap child environment credential-hygiene comment

• Rewraps the existing child environment rationale at a sentence boundary to follow the file’s 79-column convention. The wording and behavior remain unchanged.

internal/runtime/pi_extension/fullsend-agent.js

pi_run.goDescribe the Vertex provider extension as vendored +1/-1

Describe the Vertex provider extension as vendored

• Replaces the outdated “interim” description with “vendored” for the Claude-on-Vertex extension loaded by anthropic-vertex models. Runtime command construction remains unchanged.

internal/runtime/pi_run.go

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:51 PM UTC · Completed 11:02 PM UTC

Commit: a190bdf · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.97

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added the risk/low PR risk: low label Sep 4, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: low (1/5)

Details

Minimal cosmetic comment changes to two files with 6 total line edits, no protected paths, no test impact, established author, addressing review feedback on a closed issue.

@fullsend-ai-review

Copy link
Copy Markdown

Looks good to me

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge risk/low PR risk: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant