Repository navigation
fix: resolve seven HIGH re-review findings (#486) - #505
Merged
Merged
Conversation
…dy states The wave partial repeated the undo gates and the exact reset line that the wave loop's undo spawn already pins. The partial now points at the fence and keeps the two prohibitions: never a hard reset, never a push. Frees 210 bytes in each dynamic-build variant.
… on a bad or unchanged SHA The merge prompt now checks out the integration branch, records its head as baseSha, merges with --no-ff and aborts on a conflict. A failed abort returns merged true with no mergeSha. The workflow halts before Validate unless mergeSha and baseSha are both 40-hex and differ, with one fixed reason for a bad SHA and one for an unchanged head. Both SHA fields in the halt record are null, so no rejected value is copied. Comments the wave fence already states are shortened to stay within the dynamic-build size ceilings.
…helper The merge arm of the per-ticket loop sat seven block levels deep. One async helper now holds the merge, the post-merge Validate and the undo, with early returns. It returns kept, reason, halted and treeEqual and reads only agent and the wave argument names. The loop keeps the verdict check first after the engine call and now sits four levels deep. The build-mds test gains a depth guard (no loop line more than five levels below the while) and a helper guard, each with a seeded known-bad probe. Behaviour is unchanged. The refactor and a few redundant comments removed free about 310 bytes in each compiled variant. Refs #486
The undo reset to the merge commit's first parent. After a fast-forward merge that parent is the ticket's own earlier commit, so part of a failed ticket stayed on the integration branch. The undo now runs three read-only checks in order: HEAD names the integration branch, the branch head equals the merge SHA, and the merge's first parent equals the recorded baseSha. A non-zero exit or a different value means not undone. Only then does it run git reset --keep on baseSha. It returns the branch head, and the workflow counts the undo as done only when that head equals baseSha. Any other result halts the wave, and the halt record now carries baseSha. A real-git test runs the merge, baseSha and undo lines parsed from the built prompt. A merge commit over a two-commit ticket returns to baseSha with both ticket commits unreachable. A true fast-forward, a detached HEAD and a conflicting local change are each reported not undone, and an unrelated dirty file survives. The build-mds collectors pin the exact merge and reset lines and refuse a caret reset, a hard reset and a push. The report's Halted bullet names the bad or unchanged SHA halt. Refs #486
The undo fix and the wave helper cut dynamic-build to 106,930 bytes (learning on) and 103,543 bytes (learning off). Lower both ceilings, their numeric-floors rows and the command-size literals to those sizes, with no headroom, as the other rows do. No ceiling rises. Part of #486
The waves KB still said a red merge is undone by a caret-one reset. The command now records baseSha before the merge, merges with --no-ff, halts before Validate on a bad or equal SHA, and resets to baseSha only when the returned head equals baseSha. State that rule in KB-INV-5 and in the wave execution step. Step 7 of the wave PR evidence also changed: the workflow runs verify-evidence publish inline, with no Git agent spawn. Correct that line. Rule IDs stay. Set updated to 2026-10-14. Part of #486
The byte and path limits were compared with themselves, so a changed limit passed. Pin MAX_BYTES_READ, SCAN_BYTES, SKIP_FILE_BYTES and MAX_PATHS_PER_ROOT by value. Add the truncation branches the walk had no test for: a cut scan marks the root and its chain line, a spent budget marks a later root, an exact budget is not a cut, an empty import, and the exact edge of the path and file-size limits. Refs #486
Pin how the head-mismatch window and the empty-checks window behave today: a pending poll resets the empty clock, a mismatch does not carry into the empty window, a failed poll leaves both clocks as they were, and each window settles at exactly 90,000 ms and not at 89,999 ms. The cases use a virtual clock, with no call longer than 25 s. Refs #486
The head-mismatch window and the empty-checks window each spelled out the same open-the-window and compare-to-90-seconds steps. Move them into one pure settle(firstAt, now, settleMs) helper. Each window keeps its own clock, the gh call order and the exports are unchanged, and the settle comparison now appears once. A source guard checks that. Refs #486
walkRoot was 94 lines and did four jobs. Move the head scan into scanImports, which returns the tokens, the bytes read and whether the run budget cut the scan, and touches nothing outside itself. Move the root figures into summarizeMembers, a pure step after the loop that reads the members in breadth-first order. walkRoot is now 53 lines and the only function that changes the run context. The result no longer carries a members list for audit() to drop. The output and the hop check order are unchanged. Source tests check the line limit and the single writer. Refs #486
…d branch-name closing paths Step 4a now treats a supplied ISSUE_PR_LINK, "(none)" included, as the section's final answer. Only an absent link reaches step 4b, and only when no admitted wave block exists. An empty Related Issues heading is written only when ISSUE_NUMBER is set, and a set ISSUE_NUMBER with no admitted line reports the existing ambiguous-issue-reference reason. GitHub step 4b renders only the setup-task link line, through the step 4a shape check, and no longer composes a closing line from a bare number or a branch name. Tests: new cases for a supplied none, the degraded line, a composing step 4b and a heading with both keys none; a characterization that every fence passing ISSUE_NUMBER also passes ISSUE_PR_LINK; the pinned phrases are updated.
…ear step 4b Jira and Linear step 4b fell back to the branch name pattern and built a reference from it. A token in a branch name can look like an issue key and name another issue, so the PR could carry a wrong reference. Step 4b now takes the key or reference returned by setup-task and ensure-traceable-issue as its only source, says never to compose a line from a bare issue number or a branch name, and publishes the rendered line only if it passes the step 4a shape check. The fetch-by-key degradation stays as option b. The ref_preflight_branch define, its export and its shared-rules row had no other caller, so they are removed. Tests: a negative pin for GitHub, Jira and Linear (no branch-derived text, no composing step 4b), a probe per provider with a seeded branch fallback, and a check that no tracker source names the removed define.
…t the skip flag The live claude files ran with the skip-permissions flag, in the real checkout and under the real HOME, so a spawned agent could write there. createSpawnContext now builds a temp HOME and a new temp git repository (no remote, nothing copied from .git or .devflow). installDevflow runs the built init into that HOME from a second non-repository temp directory, with memory, learning, ambient, knowledge, rules and proxy off, and checks that the checkout status does not change. Both spawns run in that context with one credential, no redirect variable, the dontAsk mode, one allowed tool, a six-tool deny list and an empty strict MCP config. The prompt directly follows -p, so no list flag can take it. A live file without claude or a credential skips and prints why.
… in tests Add a static guard over tests/ that requires an env and a cwd on every claude spawn and on every node child in the integration files and the spawn-context test, and refuses the skip-permissions flag and bypassPermissions in a claude spawn and anywhere under tests/integration. Seeded probes and a mutation of every real spawn site keep it live. The ambient live file joins the preload file in the integration opt-in exclusion: both are collected only for DEVFLOW_INTEGRATION_ALL set to 1, true or yes. The gate test checks both files in each branch against the include list. The comments and one test title that described the old live-test setup now describe the temp HOME and temp repository each live file builds.
…486 Platform assumptions: the live claude tests are opt-in, run in a temp HOME and a temp repository, and use an explicit permission mode and deny list instead of the skip flag. A new row records the deny-rule assumption as unverified, because no credential was set for a probe or a live run. Compliance KB: the /code-review language focuses are gated by the install-time stamp line, never by a run-time probe or by presence. Tracker-guards KB: drop the Phase 10b clause and point to numeric-floors.json without numbers. Prompt-install KB: describe both live files, their temp HOME and repository, the permission design and the spawn guard. The skip flag stays only as a pointer to follow-up #497 for the background memory hook.
- Keep the tracker-guards KB at or below its character cap: condense three clauses of the ops paragraph. No fact is removed. - Name both live claude files in the platform-assumptions rule on the glob-time exclusion. - Give the ambient live file the opt-in run command. A plain `npm run test:integration` now excludes it. - Refer to the findings by the numbers that issue #486 uses, not by labels from the plan.
Owner
Author
Code Review — Cycle 1Full summary withheld (public repository).
Full report: .devflow/docs/reviews/fix-486-rereview-high-findings/2026-10-14_2309/review-summary.md (not committed; ask the author) |
…e helper (#486) - Check the Git-reported ticket branch in code before the merge spawn: the repo ref allowlist, no leading dash, no double dot. A refused name quarantines the ticket with a fixed reason and spawns no merge. The value is never repaired or copied. - Give the halted record a fixed kind (red, bad-sha, unchanged). The Halted line in step 3 renders fixed text per kind and no agent value, so a merge that added no commit no longer reads as a red HEAD. - Give an undo that reports done with another head a fixed reason. - Tell the Git agent to run git merge --abort when the merge stops for any reason, so a refusing commit hook no longer leaves MERGE_HEAD behind. A real-git test shows both the stuck state and the abort. - Name the merge helper in the undo doctrine and state that Validate runs after every merge except when a bad or unchanged SHA halts the wave first. - Pay for the added bytes with cuts of duplicate wording in the same files, and lower both dynamic-build size ceilings to the new sizes.
Step 4b in the GitHub, Jira and Linear references kept the shape of the removed branch fallback. GitHub named a producer that returns no Handoff Values block. Jira and Linear listed one source under a resolution order, kept a fetch-by-key gate for a call that step no longer makes, and offered a bare-key fallback that step 4a's shape check can never admit. One shared define in the tracker common module now holds the step: the link line comes only from the Handoff Values block of setup-task or fetch-issue, is published only when it passes step 4a's check, is refused in step 4a's words, and is never composed by the step. Each provider keeps its own first sentence and its rendering note; the discarded-token fallback now renders the documented default, which passes the check. The git ops steps text no longer mentions a branch-pattern check, and the common module header counts the three remaining ref pre-flight heads. The seam guard drops its fetch-by-key exception and holds all three providers to the one form, and the op-mention row for step 4b names setup-task and fetch-issue.
- The skip-reason test now passes a credential to the one path that takes one and returns text, so a reason that printed the value would fail it. - The two cleanup tests assert on the exact cwd and HOME paths the injected runner saw, not on a listing of the shared temp dir that other runs write to. - Add dispose tests: a path outside the temp root, a sibling that shares the temp-root prefix and a path that climbs out of it are refused, the other directory is still removed, and the first error is the one thrown. - Compare only the paths devflow init writes before and after an install, and print the differing status lines on failure. - The spawn guard now matches shell-string claude calls (exec and execSync), with the claude --version probe in the helper file as the only exemption.
- Give the real-git undo tests a 60 s budget above the 20 s per-call git budget, so the default 5 s test timeout cannot fire first. - Pin the fast-forward refusal to the first-parent check by its exact reason, not to any check that prints another value. - Compare the merge prompt's baseSha order only when both lines exist, so a missing --no-ff line reports one defect. Add a probe for a baseSha recorded after the merge. - Name the fake gh default call time in the CI wait test. - Add undo replies with no head and with a head that is not hex. Both halt the wave as a refused undo.
- CHANGELOG: name the ticket branch check, the halt when a merge adds no commit, the missing wave PR on a halt, and the halt reason in the run summary. A failed post-merge build is the undone case, not a conflict. - Wave KB: the first parent of mergeSha is baseSha whenever the merge adds a commit. The halted record renders fixed text per kind, and mergeSha only for kind red. State the branch check and the three halt kinds. - Test-harness KB: the live-claude tests build a temp HOME and temp repo and run init there; they do not use the real ~/.claude.
- Rename LIVE_CLAUDE_TEST to LIVE_PRELOAD_TEST so it is parallel to LIVE_AMBIENT_TEST; it names only the preload file. - Match the integration config's exclude list as globs, as include is, so a glob exclude that covers a live file is detected. A trailing ** now reads as any depth. Add cases for a glob exclude that covers a file and for one that does not. - Point the permission-mode note in both live files at the args builder that sets it, and give both the same Run note.
Owner
Author
Resolution SummaryFull summary withheld (public repository).
Full report: .devflow/docs/reviews/fix-486-rereview-high-findings/2026-10-14_2309/resolution-summary.md (not committed; ask the author) |
Owner
Author
Test Plan Evidence — 23f450bVerified 53/73: VERIFIED-CI 44, ATTESTED-LOCAL 9, UNVERIFIED 10, STALE 10, FAILED 0, INDETERMINATE 0
|
Owner
Author
Code Review — Cycle 2Full summary withheld (public repository).
Full report: .devflow/docs/reviews/fix-486-rereview-high-findings/2026-10-15_0121/review-summary.md (not committed; ask the author) |
Bring in the GATE_LEVEL settings line and machine default. The merge had no conflicts. The compiled dynamic-build command stays under both byte ceilings, so no ceiling changed.
The live-file install builds a temp HOME and a temp repository, then installs devflow into that HOME. init runs from a separate directory that is not a repository. The install fails when a path that init writes changes in the checkout.
The two-gate-shapes entry now names the rule that the claude-dir guard still holds: no prompt reads a path under the home Claude directory outside the CLAUDE_CONFIG_DIR rule.
Step 4b runs only when the PR link is absent and no wave block is admitted. It takes only the Handoff Values link line, composes no line, refuses a bad line in step 4a's words, and skips silently when there is none.
The disposeSpawnContext describe kept thrownMessage local and the checkout-status describe repeated it as an inline function. Move it to file scope and call it from both places.
Add a Changed entry for the PR link and wave ticket branch rules, and name the merge or base SHA in the Fixed entry.
The "climbs out" row built its path with path.join, which folds the ".." away before the guard sees it, so it repeated the first row. Build it as text so the segment survives. Add a row for a link under the temp root that points outside it: a guard that does not resolve links lets it through, and rmSync would then remove only the link.
The guard missed path-qualified binaries, the launchers npx, sudo,
nice, xargs, timeout and env flags, the separators &, backtick, { and
!, shell keywords such as then and do, quoted program names, and a
command string given to spawn, spawnSync, execFile or execFileSync.
Build the command-string regexes from named parts, accept a path
before the name, add the missing launchers and separators, and add the
argument-list arm. Each new form has a known-bad probe through the
guard's own collector. Forms that stay out of scope are named in the
header, and each has a "does not match" probe.
…that began (#486) The wave merge prompt told Git to run git merge --abort whenever a merge stopped. A merge that never began (a missing ref, local changes it would overwrite, unrelated histories) leaves no MERGE_HEAD, so the abort exited 128 and the wave halted as bad-sha although nothing had merged. The prompt now aborts only when git rev-parse -q --verify MERGE_HEAD succeeds, and a merge that did not begin returns merged false with the reason. The ticket branch comes from a lower-trust agent. The check now refuses a value that is not a string, a leading # (the shell reads it as a comment), and the revision names HEAD, @, FETCH_HEAD, ORIG_HEAD and MERGE_HEAD. The merge prompt verifies refs/heads/<name> first and merges that ref, so a tag, a SHA or a remote ref never merges. Tests: real-git cases for a missing ref, a tag and a SHA; refused probes for #7-x, the revision names and a branch that is not a string; a seeded probe for each collector arm, including the late check and the loose allowlist. The wave-flow describe title now states the rule instead of a review label. The byte growth is offset by cutting wave doctrine that restated the helper.
…ed record (#486) Describe the local-branch check, the abort that runs only for a merge that began, the string and allowlist check on the ticket branch, and the five fields of the halted record.
#486) The shared step 4b takes only the Handoff Values link line, composes no line and skips silently when there is none. The Jira and Linear notes still told the step to render the key or reference as the documented default when the rendering section was absent, so an agent could publish a reference the shared text forbids. Each note now covers closing effects only. The rendering gate and the documented-default paragraph in these two ensure-pr-ready references served no step and are removed; create-release keeps both. The first sentence takes the link form from pr_link_default. The section-14.3 claim rows and the render-site set in schema-scope now name create-release as the only operation that renders the token.
collectStep4bComposition now reports a step 4b block that tells the agent to render the key or the reference, or that names the rendering section. The known-bad probe seeds the removed Jira and Linear sentences and the leading rendering sentence through the same collector, and checks that each seed lands.
Owner
Author
Resolution SummaryFull summary withheld (public repository).
Full report: .devflow/docs/reviews/fix-486-rereview-high-findings/2026-10-15_0121/resolution-summary.md (not committed; ask the author) |
dean0x
added a commit
that referenced
this pull request
Oct 15, 2026
…s after #472 (#514) ## Summary PR #508 cut 76 bytes from each command that carries the settings line. It did not change the command byte ceilings. This PR lowers the ceilings to the measured sizes on main (8875e09). The slack is now 0 for each changed row. Only ceiling values change, in `tests/guards/command-size.test.ts` and `tests/fixtures/numeric-floors.json`. The `description` of each changed fixture row now states the new measured size. No prompt text changes. ## Ceilings | Command | On: old → new | Off: old → new | |---|---:|---:| | bug-analysis | 25065 → 24966 | 21077 → 21056 | | debug | 19967 → 19885 | 17046 → 16966 | | dynamic-plan | 36994 → 36880 | 32557 → 32530 | | dynamic-profile | 16487 → 16389 | 12684 → 12673 | | dynamic-tickets | 41450 → 41336 | 37156 → 37129 | | explore | 12944 → 12866 | 10040 → 9964 | | implement | 49883 → 49807 | 47029 → 46953 | | plan | 38096 → 38018 | 35126 → 35050 | | release | 22903 → 22825 | 19609 (no change, already exact) | | research | 14972 → 14894 | 10838 (no change, already exact) | | resolve | 52422 → 52287 | 49220 → 49087 | | self-review | 15889 → 15691 | 12801 → 12619 | Sizes are in bytes. ### Not changed here - `code-review` (on and off): open PR #512 changes this command and its ceilings. - `dynamic-build` (on and off): open PR #505 changes this command and its ceilings. Those PRs set their own ceilings. Open PR #513 changes no command and no ceiling. ## Evidence - `npm run build` on a clean tree: exit 0, 33 compiled, 0 errors, 0 warnings. - The measured sizes equal the slack table in the #508 PR body. There are no differences. - `npx vitest run tests/guards/command-size.test.ts tests/guards/numeric-floor-manifest.test.ts`: 2 files, 16 tests pass. - `npx tsc --noEmit`: exit 0. - CI: see the checks on this PR. ## Breaking Changes None. The change is in test constants only.
…-findings # Conflicts: # .devflow/features/compliance-feature/KNOWLEDGE.md
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
1 participant
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.
Summary
This PR fixes all seven blocking HIGH findings from the re-review of PRs #436, #440 and #459. Each finding is named as "finding N of #486".
Changes
--no-ff. The integration HEAD is recorded as baseSha before the merge. The undo resets to baseSha after read-only checks.while.git rev-parse --verify --quiet refs/heads/<name>first. The code check refuses a value that is not a string, a name with a leading#or-, a name with.., and the namesHEAD,@,FETCH_HEAD,ORIG_HEADandMERGE_HEAD. The merge aborts only when it began, which meansMERGE_HEADexists. A merge that did not begin returnsmergedfalse, and the ticket is quarantined.Breaking Changes
(none). Step 4b runs only when ISSUE_PR_LINK is absent and no wave block is admitted. It takes only the Handoff Values link line, and it refuses that line in step 4a's words when the line fails the check. It composes no line.TRACEABILITY: DEGRADED (ambiguous issue reference).Refs {KEY}-{n}. A custom## Reference Renderingno longer shapes that line. It still shapes the Shipped Issues section of a release. Follow-up fix: apply the Reference Rendering gate to the Handoff PR link line (Jira, Linear) #511 tracks this change. The maintainer accepted it.Deviations
--no-ffmerge is the covered case.Validation
QA
Not verified: deny-rule binding for subagents and the live proof (no token)
live proof not run: no token
Reviewer Focus Areas
red,bad-shaandunchanged. Each kind has fixed text. Check each text against its case.Disputed findings
None. The wording of finding 7 of #486 is corrected: walkRoot is a function of 94 lines with a loop of 55 lines. No count is disputed.
Follow-ups
Related Issues
Closes #486
Test Plan