fix: runner correctness bugs, cleanup, tests, and architecture docs - #25
Conversation
Two correctness fixes found in review:
- The pre-push path passed an empty RunOptions{} to runHookCfg, silently
ignoring --tool/--skip-tool/--group/--check/--no-cache/--verbose on
`forge run pre-push`. Thread the caller's opts through.
- stage_outputs were `git add`ed even when the tool failed, unlike restage
which already guards on success. Both runners now stage only on success.
Adds regression tests for both.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- parseAllowedGroups used a bufio.Scanner with a custom split function to do what strings.Split does in one line. - The tool `when` field was decoded but never read anywhere; remove it so the strict decoder no longer accepts a no-op key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- docs/architecture.md: request-pipeline Mermaid diagram + package map - document the .git-hooks.env / .env auto-loading behavior (was undocumented) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TerrorSquad
left a comment
TerrorSquad
left a comment
There was a problem hiding this comment.
Self-review of the diff. Fixes verified correct (pre-push now threads opts; both runners guard stage_outputs on err == nil, matching restage). One consistency gap from the when removal:
Embedded JSON schema still declares when (internal/forge/schema/forge.schema.json:161)
The struct field is gone and the strict decoder now rejects when, but the schema still advertises it — and describes it as functional ("Conditional expression controlling when this tool runs"), which it never was. An editor using the schema would suggest a key that fails forge validate. Removing it from the schema too.
Everything else checks out: --all-files is still rejected for non-pre-commit before opts are threaded, parseAllowedGroups behavior is preserved (test added), and the full suite passes.
The struct field was removed; the embedded JSON schema still advertised `when` (as a functional field it never was). Drop it so editor autocomplete can't suggest a key that fails validation. Addresses review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Follow-up to the docs work: an actual code + test review of the runner, with fixes.
Bugs fixed
RunHookWithOptionspassed an emptyRunOptions{}down the pre-push path, soforge run pre-push --tool/--skip-tool/--group/--check/--no-cache/--verboseall did nothing. Opts are now threaded through. (regression test added)stage_outputsstaged files from failed tools. Both the sequential and parallel runnersgit addedstage_outputsregardless of tool exit status — unlikerestage, which already guards on success. Now both stage only on success. (regression test added)Cleanup
parseAllowedGroupsreplaced abufio.Scanner+ custom split func with a one-linestrings.Split.whentool field (decoded but read nowhere; the strict decoder was accepting a no-op key).Tests
review_fixes_test.gocovering the two fixes andHOOKS_ONLYparsing.Docs
docs/architecture.md: request-pipeline Mermaid diagram + package map + key invariants..git-hooks.env/.envauto-loading.Verification
go build,go vet,gofmt -lclean; fullgo test ./...passes.🤖 Generated with Claude Code