Repository navigation
Conversation
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#5310): prohibit git reset --hard in the executor with no exception
Head reviewed: 6522b0a (gh pr view this turn: mergeable=MERGEABLE, mergeStateStatus=BEHIND, reviewDecision=REVIEW_REQUIRED, 36 status entries).
Summary
One-line prompt fix in agents/gsd-executor.md:597 (drops the except inside <worktree_branch_check> carve-out), plus the same stale permission removed one include-hop away in gsd-core/references/git-integration.md:243 and its zh-CN translation, a Fixed changeset, and a new regression test in tests/worktree-safety.test.cjs. Substance is correct; only minor/nit findings below.
Classification & gate compliance
- Linked issue #5310 carries
bug+confirmed-bug-> Bug-Fix track. Body hasFixes #5310; template (Fix PR) followed; not a draft. (deterministic) - Title
fix(#5310): ...conventional. Branchfix/5310-executor-reset-hard-unconditionalconforms. (deterministic) - Changeset
.changeset/graceful-badgers-run.md: type Fixed,pr: 5416backfilled, bold-lead body. OK. Fixed is docs-exempt. - Scope: issue scoped to the executor bullet plus a test covering both statements. The PR additionally edits
git-integration.md(+ zh-CN). This is the same defect (a text licensing executor hard reset) reachable viaexecute-plan.md:9@-include; judged in scope, not scope creep (judgment). - No
references//workflows/files added -> no INVENTORY drift. No CONTEXT.md vocabulary change. - PR has no labels (
labels: []). Issue is labelled; see finding M1.
Must-Have Acceptance Checklist (issue #5310 Agent Brief, verbatim)
- "No clause in the executor's prohibited list exempts
git reset --hard." -> PASS.git show refs/prrev/5416:agents/gsd-executor.mdline 597:- \git reset --hard` (no exception: `<worktree_branch_check>` is verify-only, #48); onlygit reset` mention in the file. - "The branch-check fragment is unchanged and still contains no hard-reset." -> PASS. Not in the file list;
grep -c "git reset"on head fragment = 0. - "A test covers both statements." -> PASS (see Test quality).
Root-cause / functional verdict
Root cause: bullet exemption added in #2076 when the branch check still recovered by reset; #590/#48 made the check verify-only without updating the bullet. Diff removes that clause (root cause, not symptom). The extra git-integration.md change removes a second hard-reset recommendation read by the executor through execute-plan.md:9. Verified git-integration.md on head has no reset --hard; remaining repo hits are docs/adr/612-... (historical rationale) and gsd-core/workflows/undo.md:631 (a prohibition), neither executor-licensing. The new bullet text is 4 bytes shorter and single-line per PR claim; line-pinned allowlist concern is not re-derived (CI-executed).
Blast radius
Prompt text only; no runtime path. CodeGraph shows the test file has no inbound dependents; agent prompt edit triggers full_matrix per scripts/ci-test-scope.cjs (PR body, not re-verified). next has since changed agents/gsd-executor.md (+6/-2 lines, none touching reset); PR reports MERGEABLE (no textual conflict).
Test quality
- Fail-first: on base the bullet is
- \git reset --hard` except inside the ..., so the pinned regex assertion fails; basegit-integration.mdcontainsgit reset --hard`, so the final assertion fails. Passes on head. (reasoned; CI executes) - Controls enumerated in PR body each target a distinct assertion. CRLF-safe split (
/\r?\n/). No wall-clock use. Reads.mdprompts, not.cjs/.js/.ts, solocal/no-source-grepis not triggered.WORKTREE_BRANCH_CHECK_FRAGMENTandreadhelpers exist in the file (worktree-safety.test.cjs:6392, :6394). - Not a parser/budget/bijective contract, so no fast-check requirement; no limit boundaries apply.
- zh-CN page not asserted (author states this openly).
Co-change completeness
docs/zh-CN/references/git-integration.mdupdated alongside.docs/features/git-integration.mdhas noreset --hard. No other locale copy ofreferences/git-integration.mdexists (git ls-tree). Complete.
Memtrace Evidence
- get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- Substitute evidence: CodeGraph explore on the test file;
git show/git grepagainstrefs/prrev/5416;git diff refs/prrev/5416...origin/nextfor the executor;.out-of-scopeand ADR claims taken from issue #5310 triage comment (not independently re-read).
Security lens
No file/argv/user-input code. Prompt change only reduces destructive-command permission; no injection surface, no new tags in agent docs.
Findings
Minor
- M1 (deterministic, gate): PR has no labels (
labels: []). Maintainer gate policy says unlabelled is closed without review; confirm label/no-changelog policy applies (changeset is present, so nono-changelogneeded). Fix: have a maintainer apply the track label or confirm not required. - M2 (deterministic): head is BEHIND
next(mergeStateStatus: BEHIND) andagents/gsd-executor.mdchanged onnext(6 insertions, 2 deletions; noresetlines). Fix:git fetch origin && git rebase origin/next, re-run gsd-test on the new sha.
Nit
- N1 (deterministic):
tests/worktree-safety.test.cjsend of new test is); });on one line (closing brace glued after the finalassert.ok(...), two spaces). Put});on its own line. - N2 (deterministic): commit 6522b0a
ci: re-run after next fix-forwardis an empty-purpose commit with no(#5310)scope; squash it on rebase.
Verdict
CI (collapsed latest-per-name for head 6522b0a): no non-success/non-skipped checks; no action_required runs for this head.
Changes requested (zero tolerance: M1, M2, N1, N2). Substance is clean.
|
Heads-up on a change to how fixes land in 1. No need to keep merging 2. Add a Self-test report (fix PRs). Fix PRs now carry a short 3. Review. The advisory 4. CI cost. Windows/macOS lanes now run on the queue, not on every push. If your change is platform-specific, add the Questions are welcome here. Details: docs/branch-protection.md. Posted at the maintainer's request. |
…no exception The executor's "Prohibited commands in worktree context" list exempted `git reset --hard` "inside the `<worktree_branch_check>` step at agent startup". That exception came in with open-gsd#2076, when the branch check still recovered by resetting. open-gsd#590 (fix open-gsd#48) made the check verify-only, so its fragment now forbids hard-reset outright and its bash contains no `git reset`; on any failed assertion it exits 42 and the orchestrator recovers. open-gsd#4463 declined bringing executor self-repair back. The bullet is now unconditional and points at open-gsd#48, in fewer bytes than the clause it replaces (no emitted growth) and on one line, so the line-pinned PROSE_ALLOWLIST entry below it does not shift. Same defect, one delegation hop away: execute-phase.md hands the executor execute-plan.md, which @-includes references/git-integration.md as required reading, and its "Failure recovery" notes, addressed to "Claude in next session", ended with "Can `git reset --hard` to last successful task". open-gsd#48's own test checks execute-plan.md for reset text but not its include. That line now names the last task commit as the point to resume from, without the reset; the zh-CN translation of the same line is updated to match. The new test in worktree-safety.test.cjs pins the `git reset --hard` bullet to the bare command, optionally with its current note, rather than scanning it for carve-out words: a word list admitted rewordings it did not name and rejected honest prohibitions ("not allowed, even during startup recovery"). It also asserts the executor mentions the command only in that bullet, so a re-grant in a later bullet or paragraph fails; that the branch-check fragment still declares VERIFY-ONLY and contains no `git reset` in any form (the existing open-gsd#48 assertion only rejected `git reset --hard {EXPECTED_BASE}`); and that execute-plan.md still includes git-integration.md and the latter no longer offers `git reset --hard`. Controls: the fixed, bare and CRLF executors pass; the pre-fix bullet, four rewordings, a continuation line, a trailing paragraph, a `git reset` planted in the fragment and the pre-fix git-integration.md each fail their assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHvymDVY86BMYn3GC7oVzT
6522b0a to
befe44b
Compare
|
Thanks for the review. Round 1 is pushed: the branch is rebased onto Findings
Also in this round
Reverting any one of the three fixes in isolation fails a named check: the Pi regen fails CI on Log: https://github.com/open-gsd/gsd-core/actions/runs/39934421391/job/120350439217 |
trek-e
left a comment
There was a problem hiding this comment.
Re-review: fix(#5310): prohibit git reset --hard in the executor with no exception
Head reviewed: befe44b (2 commits; mergeable=MERGEABLE, 4 commits behind next, merges cleanly per git merge-tree).
Summary
Follow-up to my 2026-10-17 round (M1 labels, M2 behind next, N1 ); });, N2 empty ci: commit). Round 1 is addressed: N1 is fixed (}); on its own line), N2 is gone (rebased to two commits), and the regenerated pi/package-agents/gsd-executor.md + manifest.json hash is a correct addition (sha256 of the pi copy equals the manifest entry; the edited region is byte-identical to agents/gsd-executor.md). M2 no longer applies: with the merge queue, "behind next" is not a prerequisite (see the earlier maintainer note on this PR), and the branch merges cleanly. M1 is a maintainer-side label action, not a contributor defect.
Review findings: 0 Blocker, 0 Major, 0 Minor, 0 Nit on this diff.
CI state for this exact head: not green. conformance test (windows-latest, 24, shard 2/3) and Required tests are red; Reviewed or maintainer is red until a maintainer review lands. So this review is a comment, not an approval (never approve a SHA whose CI I have not seen finish green).
Classification & gate compliance
- Linked issue #5310 (
bug,confirmed-bug, open) -> Bug-Fix track.Fixes #5310present, fix template used, not a draft. (deterministic) - Changeset
.changeset/graceful-badgers-run.md:type: Fixed,pr: 5416. (deterministic) RULESET.CONTRIB.CLASSIFY.fix=requires confirmed-bug before implementationsatisfied.- Self-test report check: success on this head.
Root-cause verdict
Root cause is the stale bullet git reset --hard except inside the <worktree_branch_check> step at agent startup (agents/gsd-executor.md), left over after #48 made the branch check verify-only. The diff removes the carve-out (cause, not symptom) and removes the second hard-reset hint reachable through execute-plan.md:9 -> references/git-integration.md (and its zh-CN copy). Repo-wide sweep (agents/, gsd-core/, commands/, pi/, docs/ non-ADR): the only remaining executor-facing reset --hard is the new prohibition; undo.md:631 is a prohibition, worktree-branch-check.md:21 forbids it, pr-branch.md:468 (git reset -q --hard HEAD) belongs to the pr-branch command, not the executor (author flagged it out of scope; I agree). /root-cause-analysis has no installed skill; the root-cause comparison above was done manually.
Test quality
New test in tests/worktree-safety.test.cjs (reads .md prose, not source .cjs/.js/.ts, so local/no-source-grep is not triggered). Fail-first, static: the pinned anchored regex rejects the pre-fix bullet, and the pre-fix git-integration.md contains git reset --hard, so both assertions fail on base. CRLF safe. No wall-clock use. Not a parser/budget contract (no fast-check requirement). Stated limit (the check is a literal match and will not catch a respelling such as git reset -q --hard) is disclosed in the PR body; I treat it as accepted scope, not a defect.
Blast radius & risk tier
Prompt text plus one prose-pinning test; no runtime code path. Risk: low. find_central_symbols / find_bridge_symbols (graph head is older than next): no changed symbol appears; the new test lives inside the large describe block in tests/worktree-safety.test.cjs, a file that does contain bridge-ranked helper functions (execGit copies, anonymous describe bodies), but the addition only reads files.
Co-change completeness
get_cochange_context lists gsd-core/workflows/execute-phase.md, execute-plan.md, CONTEXT.md, docs/INVENTORY.md as historical partners of the executor prompt; none needs an update here (no file added to references/ or workflows/, no vocabulary change, execute-plan.md only includes the edited reference). git-integration.md partners are bulk edits from 2026-08-30. zh-CN copy updated; no other locale has the file.
Memtrace Evidence
get_impact: targetWORKTREE_BRANCH_CHECK_FRAGMENTreturnedambiguous, riskUNKNOWN(two unrelated test constants); the PR's own test file symbols are not in the indexed graph (graph head 0ce32128b3 onnext, indexed before the PR). No blast-radius number is claimed; markdown-only edits have no call edges.get_symbol_context:found:falseforWORKTREE_BRANCH_CHECK_FRAGMENTintests/worktree-safety.test.cjs(not indexed at that path/branch). Partial evidence.recall_decision: query on the executor reset prohibition returned only unrelated/low-relevance items (kind: decision, none about worktree reset);governing_ruleson the three main files returned only generic ADR-443. This is not a clean "no governing decision"; it means none was surfaced.find_code_review_issues(review_mode: online,max_candidates: 50, graph_mode strict): 0 raw, 0 filtered (_graph_state: ready). Same input tofind_cross_module_issues: 0 issues (_graph_state: ready),find_yaml_rule_matches: 0,review_github_pr(post:false,minSeverity: low): 0 comments.find_ast_review_issues: N/A, no.pyin the diff. Coverage caveat: the diff passed to these tools contained only the.cjstest hunk (code comments inside it elided); the.md,.jsonand changeset files are not covered by any AST/YAML detector.detect_changes(full.md+ test hunks): 7 files mapped, 0 affected symbols (no indexed symbols).find_dead_code,find_hotspots,get_function_quality_metrics: returned only unrelated repo-wide results or no match for the changed (anonymous test) callback; nothing attributable to this PR.get_style_fingerprinton the test file: no delta from codebase norm.get_cochange_context: see above. Fleet: 0 peers, 0 conflicts for this touched set.
Laws that apply (/skills-from-the-artificer, preset bugfix-review)
Subject: the executor's prohibited-commands bullet loses its stale startup exemption, the second reachable hint in git-integration.md is removed, and a prose-pinning test guards both.
- hyrums-law — fired: the test pins the exact bullet text (
/^- \git reset --hard`( (no exception: ...))?$/`), so a later harmless rewording fails; the PR documents this as intentional. Action: none. - leaky-abstractions — fired weakly: the guard is a literal text match on prose, so a respelling escapes; disclosed limit. Action: none (maintainer may later widen to
/git\s+reset\b[^\n]*--hard/). - kernighans-law, kerckhoffs-principle, postels-law, knuths-optimization-principle — cleared: no clever code, no secrecy dependence, no parser boundary, no optimization.
Action items: none.
Skills actually invoked for this review
diagnosing-bugs, qa-test-architect, skills-from-the-artificer, gsd-test-verify, code-review, security-review. /root-cause-analysis has no installed skill (done manually). /code-review's two parallel sub-agents and /security-review's sub-tasks were performed as a direct reading by the reviewer plus one isolated adversarial subagent (maintainer-reviewer), not as nested spawns. /security-review: prompt-text-only change that only removes a destructive-command permission; no vulnerability above the confidence bar.
Isolated adversarial pass
Verdict minor: no residual executor license found; pi copy hash matches manifest. Its two observations are the literal-match limit above (disclosed in the PR body) and a suggestion to confirm a parity test guards the pi copy; the latter is covered by gen-pi-package-skills.cjs --check (part of lint:generated-sync) per the PR body and passes in CI on this head.
CI attribution (head befe44b)
Latest-per-name non-success: conformance test (windows-latest, 24, shard 2/3), Required tests (fails because of the shard), Reviewed or maintainer (advisory, resolves with a maintainer review), plus skipped Publish emitted-baseline artifact, auto-merge, test (inert CI). The red shard failed in tests/commit-files-pathspec.test.cjs ("a timed-out MERGE_HEAD probe (posix) ..."), in fixture setup: git add -- .planning/shared.md exited 128, fatal: Unable to create '.../.git/index.lock' (run 39934421391). That file is not in this PR's changed-file list and the case builds its own temp repo; the shard's other test chunks pass (# fail 0). The root cause of the lock collision is not proven by me (a re-run is blocked by this session's hooks until a fix lands), so I am not calling it "base drift", only "not attributable to the diff by file overlap". Separate evidence: my own gsd-test run of this head against next (linux-node24, base bc5dfc2) finished failed with 54307 passed / 3 failed, all three in tests/npm-integrity-gate.test.cjs (npm audit --omit=dev introduces no NEW advisories, its folded duplicate, and root workspace production tree introduces no new advisories). This PR touches no package*.json, and the lockfile on the head already carries ip-address 10.7.4, so the diff cannot have introduced an advisory; this matches the known no-baseline advisory state on next but I did not run that file on next itself, so base-drift attribution is inferred, not proven. The worktree-safety test added here passed in that run. This statement is about checks only; the review above carries 0 findings.
Verdict
Comment, no approval: 0 findings on the diff, but CI for this exact head has a red required check that I could not clear. Next step is a maintainer re-run of the failed Windows shard (or the merge queue, which re-tests on top of next anyway). No change is requested from the contributor on the diff; the 4-commit lag behind next is not required to be fixed (merge queue).
Fix PR
Linked Issue
Fixes #5310
What was broken
The executor's Prohibited commands in worktree context list allowed
git reset --hard"inside the<worktree_branch_check>step at agent startup". That step has been verify-only since #48: the fragment forbids hard-reset outright, its bash contains nogit reset, and every failed assertion ends inexit 42so the orchestrator recovers. The issue described the bullet as the last text still giving the executor a hard-reset permission; one more turned up one include away (below).gsd-core/agents/gsd-executor.md
Lines 593 to 605 in 433a434
gsd-core/gsd-core/references/worktree-branch-check.md
Lines 17 to 44 in 433a434
What this fix does
The bullet now reads:
It is unconditional and points at the reason. The neighbouring
git update-refbullet already carries the HALT-and-surface rule, so the bullet doesn't restate the exit-42 path. The new text is 4 bytes shorter than the clause it replaces, soemitted-attributionsees no growth and no ack trailer is needed. It stays on one line, so the line-pinnedPROSE_ALLOWLISTentry for the executor's line 832 doesn't move (it was line 828 when this PR was opened; #4799 onnextshifted it since):gsd-core/agents/gsd-executor.md
Line 832 in 7d51c30
Same defect, one delegation hop away
execute-phase.mdhands the executorexecute-plan.md, andexecute-plan.md@-includesreferences/git-integration.mdas required reading:gsd-core/gsd-core/workflows/execute-plan.md
Lines 5 to 10 in 433a434
Its Failure recovery notes are written to "Claude in next session" and ended with "Can
git reset --hardto last successful task":gsd-core/gsd-core/references/git-integration.md
Lines 240 to 243 in 433a434
That line now reads "Task 1's commit is the known-good point to resume from". #48's own test (
tests/worktree.test.cjs,execute-plan.md worktree mandate is verify-only) checksexecute-plan.mdfor reset text, but not the file it includes. The issue said the prohibited-list bullet was the last text licensing an executor hard reset. That was one file short.No change to
worktree-branch-check.mdor to the orchestrator's recovery path. This doesn't reopen #4463 (.out-of-scope/executor-self-repair-worktree-base.mdstays as it is).Root cause
The exception came in with 5c0e801 (#2076, the
git cleanprohibition), when the branch check still recovered by resetting. #590 (fix #48) made the check verify-only but left this bullet alone.Testing
How I verified the fix
A new test in
tests/worktree-safety.test.cjssits in the #3384 block, next to the existing #48 fragment test. It checks both statements from the triage brief, plus the include:git reset --hardbullet. That bullet is pinned: the bare command, optionally followed by its current(no exception: …)note. It also requiresgit reset --hardto appear only once ingsd-executor.md, so a re-grant in a later bullet or paragraph fails too.VERIFY-ONLYand contains nogit resetin any form. The existing execute-phase.md: split worktree lifecycle between orchestrator and sub-agent; removegit reset --hardrecovery #48 assertion rejects only the literalgit reset --hard {EXPECTED_BASE}.execute-plan.mdstill includesgit-integration.md, andgit-integration.mdno longer containsgit reset --hard.I ran negative controls against the same test, then restored the tree:
- `git reset --hard`(the issue's short form)except inside the <worktree_branch_check> step at agent startup)(startup recovery is authorized)(no exception except at startup)startup recovery may still use itunder the bulletStartup recovery may run `git reset --hard` to repair the base.# git reset --hard HEADappended to the fragmentgit-integration.mdWhy pinned rather than scanned for carve-out words: an earlier draft of this test rejected words like
except,unless,allowedandpermitted. Driven against 69 rewordings, 60 exemptions still passed (may,is fine,other than,save for, …), while honest prohibitions such as "not allowed, even during startup recovery" failed. Rewording the bullet now means editing this test, which is a reasonable checkpoint for a safety rule.Limits, stated plainly: this guards against the original exemption coming back and against a re-grant that spells out
git reset --hard. It does not catch a deliberate rewording that avoids the literal command ("perform a hard reset",git -C … reset --hard, the list fenced off as an old example), and no text match on prose will. Review covers that. The single-mention check also fails if someone adds another honest line naming the command elsewhere in the file. That edit would have to update this test too.Sibling sweep: I grepped
reset --hardacrossagents/,gsd-core/,commands/anddocs/, then followed the executor's required reading out of the orchestrator's prompt. The only other hit that reaches the executor was thegit-integration.mdline above.docs/zh-CN/references/git-integration.mdcarried a translation of the old line. No agent loads it, but it would otherwise still recommend the recovery #48 ruled out, so its line is translated to match. The test doesn't assert on it.worktree-safety.test.cjsis docs-guard-exempt, so covering the page would mean dropping that exemption and registering this 440-test file in the docs-guard lane. I didn't think that was worth it for a page no agent loads. Happy to add it if you'd rather have the coverage.Since #5569 landed on
next,pi/package-agents/gsd-executor.mdis a generated copy of the executor prompt, and it carried the same exception. The fix commit now includes the regenerated copy and itspi/package-agents/manifest.jsonhash, andnode scripts/gen-pi-package-skills.cjs --check(part oflint:generated-sync) passes at both commits. Re-sweepingagents/,gsd-core/,commands/,docs/andpi/for any spelling ofreset … --hardturns up, besides the executor's own prohibition,undo.md's prohibition and ADR 612's historical rationale, one more instruction:gsd-core/workflows/pr-branch.md(git reset -q --hard HEAD). That workflow belongs to the pr-branch command (/gsd:pr-branch, also reached throughns-manageand the Pigsd-pr-branchskill), not the executor, and it resets that command's own disposable branch, so it is outside this issue.scripts/ci-test-scope.cjssetsfull_matrix=truefor an agent-prompt edit. Before opening this PR I ranbuild:lib,lint:ci,lint-default-flip-documentation, and the unit, integration, security, install, slow and qa suites, in a throwawayHOMEwithCLAUDE_CONFIG_DIRunset. The coverage-instrumented shards and the coverage gate are left to CI.Regression test added?
Platforms tested
Runtimes tested
Self-test report
{ "schema": 1, "head_sha": "befe44baac72e5c3e6cba18e65e1956651109c6f", "tool": "npm-test", "failing_first": { "test": "tests/worktree-safety.test.cjs", "fails_on_base": true, "passes_on_head": true }, "commands": [ "node --test tests/worktree-safety.test.cjs", "node scripts/gen-pi-package-skills.cjs --check", "npm run lint:ci", "npm run test:coverage:unit:raw -- --shard 1/3 (and 2/3, 3/3)", "npm run test:integration", "npm run test:security", "npm run test:qa", "npm test" ], "full_suite": { "command": "npm test", "result": "passed", "passed": 46523, "failed": 0 }, "environment": { "os": "Ubuntu 24.04.4 LTS on WSL2 (kernel 6.18.54.1-microsoft-standard-WSL2)", "node": "v24.21.0" }, "output_excerpt": "# tests 2118\n# suites 399\n# pass 2118\n# fail 0\n# cancelled 0\n# skipped 0\n# todo 0\n# duration_ms 47986.373633\nrun-tests: chunk 29/29 completed in 48107ms\n(npm test exit code 0)\n(29/29 chunks completed; summed over all chunks: tests 46632, pass 46523, fail 0, skipped 108, todo 1)" }Checklist
Fixes #NNN— PR will be auto-closed if missingconfirmed-buglabelnpm test).changeset/fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — orno-changeloglabel appliedBreaking changes
None for code or output. The instructions change: the executor loses a permission that the step it named had nothing to apply to, and loses a recovery hint that #48 had already ruled out for it.