Skip to content

fix(#5761): resolve source dir before dry-run output - #5763

Merged
ifireball merged 3 commits into
mainfrom
agent/5761-fix-dryrun-source-dir
Aug 3, 2026
Merged

fix(#5761): resolve source dir before dry-run output#5763
ifireball merged 3 commits into
mainfrom
agent/5761-fix-dryrun-source-dir

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Fixes mint deploy --dry-run reporting "embedded" source when the actual deploy would use the checkout path. Moves sourceDir default resolution before the dry-run block in both GCP and Cloudflare deploy functions. Updates --source-dir flag help text to describe the actual defaulting behavior.

Related Issue

Closes #5761

Changes

  • Move sourceDir default resolution (DefaultFunctionSourceDir / DefaultWorkerSourceDir) to before the dry-run early return in runMintDeployGCP and runMintDeployCloudflare
  • Simplify dry-run source output to always show the resolved sourceDir value (no more conditional embedded vs explicit path)
  • Update --source-dir flag usage text from (default: embedded) to (default: checkout path when present, embedded otherwise)
  • Add four new tests verifying dry-run output shows the resolved checkout path for both GCP and Cloudflare platforms, and that explicitly provided --source-dir values are displayed correctly

Testing

  • All mint-related tests pass (go test ./internal/cli/... -run TestMint)
  • New tests verify dry-run output contains the default checkout path and does not contain "embedded" strings
  • New tests verify explicitly provided --source-dir values are shown correctly in dry-run output
  • Existing dry-run tests continue to pass

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Not a breaking change (dry-run output is informational, not machine-parsed)

Closes #5761

Post-script verification

  • Branch is not main/master (agent/5761-fix-dryrun-source-dir)
  • Secret scan passed (gitleaks — 2843dc797bbd8b74d72b5984023aa216db73c147..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

The dry-run code path in both runMintDeployGCP and
runMintDeployCloudflare printed source information and returned
early before the sourceDir default was resolved. This caused
dry-run to report "embedded mint function" / "embedded Worker
adapter" even when a real deploy from the same checkout would
use the on-disk checkout path (internal/mint for GCP,
internal/dispatch/cf/workersrc for Cloudflare).

Move the sourceDir default resolution (DefaultFunctionSourceDir /
DefaultWorkerSourceDir) to before the dry-run block in both
functions so dry-run output matches real deploy behavior. Also
update the --source-dir flag help text to describe the actual
default (checkout path when present, embedded otherwise).

Note: pre-commit could not run in sandbox (network restriction);
post-script will run it authoritatively.

Closes #5761
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner July 30, 2026 14:16
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Jul 30, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 30, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:17 PM UTC · Completed 2:30 PM UTC
Commit: 8eb5d2a · View workflow run →

@codecov

codecov Bot commented Jul 30, 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 commented Jul 30, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [edge-case] internal/cli/mint.go:510DefaultFunctionSourceDir() returns the relative path internal/mint unconditionally. The new os.Stat guard correctly handles the common cases (running from a repo checkout vs. standalone binary), but in the unlikely scenario where the cwd happens to contain an internal/mint directory that is not a fullsend checkout, dry-run would report it as the source directory. The same applies to DefaultWorkerSourceDir() in the Cloudflare path. This is a pre-existing limitation — the PR improves the situation over the previous behavior (which always said "embedded" regardless).
Previous run

Review

Findings

Low

  • [edge-case] internal/cli/mint.go:493DefaultFunctionSourceDir() returns internal/mint unconditionally without checking whether the directory exists on disk. When the CLI runs as a standalone binary outside a repo checkout, dry-run will display this relative path as the source directory even though the actual deploy would fall back to embedded source. The same applies to DefaultWorkerSourceDir() in the Cloudflare path. This is an improvement over the previous behavior (which always said "embedded" regardless of context) but is not fully accurate for the standalone-binary case.

  • [misleading-default-value] docs/guides/infrastructure/mint-administration.md:128 — The Default column shows "(checkout path)" which suggests the default is always the checkout path, but the description correctly notes it's conditional. The Default column should be left empty (like in docs/cli/mint.md line 74) or use a value like "(conditional)" to reflect the varying behavior.

Previous run

Review

Findings

Medium

  • [stale-default-value] docs/cli/mint.md:74 — The --source-dir flag description currently says "Path to local mint source (default: embedded)" but the PR changes the CLI help text to describe the actual defaulting behavior (checkout path when present, embedded otherwise). The documentation will be stale after merge.
    Remediation: Update to match the new help text.

  • [stale-default-value] docs/guides/infrastructure/mint-administration.md:128 — The --source-dir flag table entry shows "(embedded)" in the Default column, which will be inconsistent with the updated CLI help text and actual runtime behavior after merge.
    Remediation: Update the Default column to reflect the new behavior.

Low

  • [edge-case] internal/cli/mint.go:493DefaultFunctionSourceDir() returns internal/mint unconditionally without checking whether the directory exists on disk. When the CLI runs as a standalone binary outside a repo checkout, dry-run will display this path even though the actual deploy would fall back to embedded source. This is an improvement over the previous behavior (which always said "embedded" regardless of context) but is not fully accurate for the standalone-binary case.

Labels: PR fixes a bug in the mint CLI deploy command (Go code)


Previous run

Review

Findings

Low

  • [edge-case] internal/cli/mint.go:493DefaultFunctionSourceDir() returns internal/mint unconditionally without checking whether the directory exists on disk. When the CLI runs as a standalone binary outside a repo checkout, dry-run will display this relative path as the source directory even though the actual deploy would fall back to embedded source. The same applies to DefaultWorkerSourceDir() in the Cloudflare path. This is an improvement over the previous behavior (which always said "embedded" regardless of context) but is not fully accurate for the standalone-binary case.

  • [misleading-default-value] docs/guides/infrastructure/mint-administration.md:128 — The Default column shows "(checkout path)" which suggests the default is always the checkout path, but the description correctly notes it's conditional. The Default column should be left empty (like in docs/cli/mint.md line 74) or use a value like "(conditional)" to reflect the varying behavior.

Previous run (2)

Review

Findings

Medium

  • [stale-default-value] docs/cli/mint.md:74 — The --source-dir flag description currently says "Path to local mint source (default: embedded)" but the PR changes the CLI help text to describe the actual defaulting behavior (checkout path when present, embedded otherwise). The documentation will be stale after merge.
    Remediation: Update to match the new help text.

  • [stale-default-value] docs/guides/infrastructure/mint-administration.md:128 — The --source-dir flag table entry shows "(embedded)" in the Default column, which will be inconsistent with the updated CLI help text and actual runtime behavior after merge.
    Remediation: Update the Default column to reflect the new behavior.

Low

  • [edge-case] internal/cli/mint.go:493DefaultFunctionSourceDir() returns internal/mint unconditionally without checking whether the directory exists on disk. When the CLI runs as a standalone binary outside a repo checkout, dry-run will display this path even though the actual deploy would fall back to embedded source. This is an improvement over the previous behavior (which always said "embedded" regardless of context) but is not fully accurate for the standalone-binary case.

Labels: PR fixes a bug in the mint CLI deploy command (Go code)

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/mint Token mint and cross-boundary credentials bug go Pull requests that update go code labels Jul 30, 2026
@ifireball

Copy link
Copy Markdown
Member

/fs-fix AI review comments

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 12:37 PM UTC · Completed 12:43 PM UTC
Commit: 8eb5d2a · View workflow run →

Update the --source-dir flag description in docs/cli/mint.md and
docs/guides/infrastructure/mint-administration.md to match the updated
CLI help text from PR #5763. The docs previously said "(default:
embedded)" but the actual defaulting behavior is "checkout path when
present, embedded otherwise".

Addresses review feedback on #5763
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Fixed 2 stale documentation references to the --source-dir flag default. Both docs/cli/mint.md and docs/guides/infrastructure/mint-administration.md now reflect the updated defaulting behavior (checkout path when present, embedded otherwise). Disagreed with 1 low-severity edge-case observation that the reviewer acknowledged as an improvement over previous behavior.

Fixed (2):

  1. stale-default-value in docs/cli/mint.md (docs/cli/mint.md): Updated --source-dir flag description from '(default: embedded)' to '(default: checkout path when present, embedded otherwise)' to match the updated CLI help text
  2. stale-default-value in docs/guides/infrastructure/mint-administration.md (docs/guides/infrastructure/mint-administration.md): Updated --source-dir Default column from '(embedded)' to '(checkout path)' and description to '(default: checkout path when present, embedded otherwise)' to match the updated CLI help text

Disagreed (1):

  1. edge-case: DefaultFunctionSourceDir returns path unconditionally: The reviewer marked this as low severity and explicitly noted it is 'an improvement over the previous behavior.' The observation is that the standalone binary case could show a path that does not exist on disk, but this is an edge-case improvement for a future PR, not a regression introduced by this change. No code fix needed.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 12:44 PM UTC · Ended 12:57 PM UTC
Commit: 227bab6 · View workflow run →

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Site preview

Preview: https://871d024b-site.fullsend-ai.workers.dev

Commit: cc01435bdaa02e3384a41f4034913b21929e1e0b

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Aug 2, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:44 PM UTC · Completed 12:57 PM UTC
Commit: 227bab6 · View workflow run →

@ifireball

Copy link
Copy Markdown
Member

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Fix · ⚠️ Cancelled · Started 6:07 AM UTC · Ended 6:08 AM UTC
Commit: 227bab6 · View workflow run →

@ifireball

Copy link
Copy Markdown
Member

/fs-fix the agent review comments and the merge conflicts

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 6:08 AM UTC · Completed 6:20 AM UTC
Commit: 227bab6 · View workflow run →

Merge conflicts in runMintDeployCloudflare resolved: integrated
the preview-alias string refactor from main with the sourceDir
early-resolution from this PR branch.

Addresses review feedback:
- Add os.Stat check for default source directory in dry-run output:
  when the default checkout path does not exist on disk (standalone
  binary case), dry-run now shows "embedded" instead of the
  non-existent relative path. Explicit --source-dir values are
  always shown regardless of existence.
- Fix misleading default column in mint-administration.md: changed
  from "(checkout path)" to empty, matching docs/cli/mint.md.
- Add tests for the embedded-fallback case (path missing) for both
  GCP and Cloudflare platforms.

Addresses review feedback on #5763
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Resolved merge conflicts with main (preview-alias refactor) and addressed all 3 review findings: added os.Stat existence check for default source directory in dry-run output (both GCP and Cloudflare), fixed misleading docs default column, and added new tests for the embedded-fallback case.

Fixed (3):

  1. merge conflicts with main (internal/cli/mint.go): Resolved merge conflicts in runMintDeployCloudflare: integrated the preview-alias string refactor from main (effectiveName computation, previewAlias validation, preview URL output) with the sourceDir early-resolution from this PR branch.
  2. DefaultFunctionSourceDir/DefaultWorkerSourceDir return paths without checking directory existence (internal/cli/mint.go): Added os.Stat check in both GCP and Cloudflare dry-run blocks: when sourceDir is resolved from the default (not explicitly provided by the user), the dry-run output now checks whether the directory exists on disk. If missing, it shows 'embedded' instead of the non-existent path. Explicit --source-dir values are always shown regardless. Added new tests for both platforms covering the path-missing case.
  3. misleading default column value in mint-administration.md (docs/guides/infrastructure/mint-administration.md): Changed the Default column for --source-dir from '(checkout path)' to empty, matching the format in docs/cli/mint.md.

Tests: passed

Decision points
  • Chose to add os.Stat check in the dry-run blocks of mint.go rather than modifying DefaultFunctionSourceDir/DefaultWorkerSourceDir functions (alternatives: Modify DefaultSourceDir functions to check os.Stat and return empty string; rationale: Keeps the DefaultSourceDir functions as simple path-returning utilities without I/O side effects. The os.Stat check is only relevant for dry-run display, not for the actual deploy path. This avoids changing behavior for other callers.)
  • Track explicit vs default sourceDir with a boolean flag rather than checking sourceDir emptiness after resolution (alternatives: Pass separate parameters to distinguish explicit from default; rationale: The explicitSourceDir boolean captures the user's intent at the point of resolution, keeping the conditional logic in the dry-run block clean and readable.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:21 AM UTC · Completed 6:32 AM UTC
Commit: cc01435 · View workflow run →

@ifireball
ifireball enabled auto-merge August 3, 2026 06:33
@ifireball
ifireball added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit 0251e7d Aug 3, 2026
18 checks passed
@ifireball
ifireball deleted the agent/5761-fix-dryrun-source-dir branch August 3, 2026 06:46
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 3, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 6:48 AM UTC · Completed 7:03 AM UTC
Commit: cc01435 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #5763 — fix(#5761): resolve source dir before dry-run output

Timeline

Time (UTC) Event
Jul 30 13:58 Issue #5761 filed
Jul 30 14:00 Triage (run) correctly identifies root cause in 4 min
Jul 30 14:04 Code agent (run) opens PR — updates logic + CLI help text but misses docs
Jul 30 14:16 Review (run): 2 medium (stale docs), 1 low (edge-case, observation only)
Aug 2 12:36 Human triggers /fs-fix AI review comments
Aug 2 12:40 Fix (run, $1.37): fixes 2 doc findings, disagrees with low edge-case
Aug 2 12:43 Re-review (run): same edge-case now includes concrete suggested fix; new misleading-default-value finding
Aug 3 06:07 Human triggers /fs-fix the agent review comments and the merge conflicts
Aug 3 06:17 Fix (run, $4.01): resolves merge conflicts, adds os.Stat check, fixes docs, adds tests
Aug 3 06:20 Final review (run): 1 low remaining (pre-existing limitation), approves
Aug 3 06:32 Human approves, PR merged

What went well

  • Triage was excellent — root cause identified in 4 minutes, recommendation matched the final implementation exactly.
  • Code agent delivered a solid initial fix — correct logic change, 4 tests, updated CLI help text.
  • Review agent caught real issues — stale documentation would have shipped without the review.
  • Final quality was high — 6 tests, robust edge-case handling with os.Stat, accurate docs across 4 files.

Rework analysis

2 fix-review cycles were needed (3 commits). Two root causes:

  1. Code agent didn't update docs alongside CLI help text — changed the --source-dir flag's help string in Go but missed docs/cli/mint.md and docs/guides/infrastructure/mint-administration.md. No AGENTS.md rule links CLI flag changes to doc updates. See proposal below.

  2. Fix agent disagreed with a valid finding, then implemented it when re-asked — the first review flagged the edge case as an observation without a suggested fix; the fix agent cited this to justify disagreeing. The re-review added an explicit "Suggested fix:" section, and the second fix agent implemented it. The presence/absence of concrete remediation directly influenced the decision.

Evidence for existing issues (proposals skipped)

  • agents#372: Re-review added a concrete suggested fix absent from the first pass — direct example of the remediation inconsistency this issue tracks.
  • agents#468: Fix agent's disagree-vs-implement decision was driven by remediation wording (observation-only → disagreed; explicit suggested fix → implemented).
  • #4970: Re-review surfaced a new "misleading-default-value" finding not in the first review of the same code region.
  • #1322: This PR is a concrete instance of CLI flag semantics drifting from documentation.

Autonomy observations

The review agent's findings were all legitimate — the human reviewer added no findings the agent missed. The human's value was orchestration (triggering fixes) and judgment (overriding the fix agent's disagreement). Positive signal for review autonomy on CLI bug-fix PRs with doc updates.

Other notes

E2E failure on Aug 2 (run) was flaky — test infrastructure issue in halfsend-05 test org, unrelated to PR changes. Passed on next run without code changes.

Proposals filed

ifireball added a commit to ifireball/fullsend that referenced this pull request Aug 3, 2026
Keep early source-dir resolution for dry-run from fullsend-ai#5763 and deploy-commit
stamping from this branch; retain both sets of deploy tests.

Signed-off-by: Barak Korren <bkorren@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
rh-hemartin pushed a commit that referenced this pull request Aug 27, 2026
CLI reference pages under docs/cli/ are manually maintained and
silently drift when flag help text, defaults, or behavior change
in internal/cli/. PR #5763 demonstrated this: the code agent
updated a flag's help text but missed the corresponding docs,
requiring a review-fix cycle.

Add a bullet to AGENTS.md's "How to work in this repo" section
instructing contributors to update docs/cli/ and search
docs/guides/ when modifying CLI flags. This follows the same
pattern as the interface documentation sync rule added in #5307.

Closes #5848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug component/mint Token mint and cross-boundary credentials go Pull requests that update go code ready-for-merge All reviewers approved — ready to merge ready-for-review Triggers review agent dispatch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mint deploy --dry-run reports embedded source when checkout would be used

1 participant