Repository navigation
ORB-164: register git-guardrails on PowerShell and carry the external-interface and never-admin-merge rules in AGENTS.md - #442
Merged
Conversation
The only PreToolUse matcher named "Bash", so the identical git command issued
through the PowerShell tool fired no hook at all and git-guardrails.mjs, the
local branch protection this repository relies on, was silently absent there.
Verified live in this worktree, both refused after the change:
PreToolUse:PowerShell hook error: BLOCKED git command (Orbit git workflow):
git push origin main
PreToolUse:Bash hook error: BLOCKED git command (Orbit git workflow):
git push origin main
The refusal echoing the command back confirms the PowerShell tool populates
tool_input.command, the field the hook reads, so no payload change was needed.
Also removes the two em dashes the hook carried; tools/check-dashes.mjs skips
.claude/ so the ban was never enforced on them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
…AGENTS.md Codex workers launched into an orbit-api worktree read this repository's AGENTS.md, so orbit-ui-mobile PR #659's "Never assume an external interface" rule and its review-rule counterpart never reached them. Added here, adapted to this repository's externals (dotnet, NuGet) and its review-rule numbering. Adds the never-admin-merge prohibition from the harness repair specification section J3a alongside the existing branch-protection guardrail: no gh pr merge --admin, no direct PUT on the pulls merge endpoint, and no GraphQL mergePullRequest mutation. Naming both raw API calls is the point; forbidding only the CLI flag leaves the API paths open. If a merge genuinely needs the admin override, the agent stops and asks Thomas to merge it himself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0175eae63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+33
to
+35
| - Never perform an admin merge, in any shape: no `gh pr merge --admin`, no direct | ||
| `PUT /repos/{owner}/{repo}/pulls/{number}/merge`, and no GraphQL `mergePullRequest` | ||
| mutation. Naming the two raw API calls is deliberate; forbidding only the CLI flag |
There was a problem hiding this comment.
When a worker invokes gh pr merge --admin or either raw API form, this prohibition remains advisory: the repository-wide search finds no enforcement, and git-guardrails.mjs immediately allows commands without the word git (a direct hook invocation with the gh command exits 0). Because an admin merge bypasses the protections that could reject it after execution, add a pre-execution gate covering all three listed forms rather than placing this rule among guardrails described as mechanically enforced.
AGENTS.md reference: AGENTS.md:L26-L29
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #442 (thomasluizon/orbit-api)
Scope: PR #442 — AGENTS.md, .claude/settings.json, .claude/hooks/git-guardrails.mjs (ORB-164)
Recommendation: APPROVE
Summary
Documentation/tooling-only PR. It registers the existing git-guardrails.mjs PreToolUse hook on the PowerShell matcher (previously Bash-only, so the identical git command issued through PowerShell fired no guard at all), and adds two AGENTS.md sections: a never-admin-merge prohibition and a "never assume an external interface" verification rule adapted from the sibling orbit-ui-mobile repo. No src/, DTO, controller, or packages/shared surface is touched, so backend-hard-rules, security-reviewer, and contract-aligner gates never fire. The PR body is unusually well evidenced (live hook-trip transcripts for both Bash and PowerShell, check-dashes/check-frontmatter/node --check output, a recorded branch-protection payload). No Critical/High findings survived review.
Findings
Critical / High / Medium
None.
Low / Info
.claude/hooks/git-guardrails.mjsalso removes the file's two pre-existing em dashes (file:74, a user-facing message;file:79, a comment).tools/check-dashes.mjsexplicitly skips the whole.claude/tree, so this was never CI-required — a voluntary fix in exactly the domain (git-workflow enforcement) that upholds the rule. No action needed.- The new "never assume an external interface" rule is dogfooded by this PR itself: the PowerShell-matcher claim is backed by a live transcript in the PR body showing the hook actually tripping through the PowerShell tool, not an assumption from
--help/memory.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | N/A — gate did not fire (no src/ code changed) |
| contract-aligner | N/A — gate did not fire (no DTO/route/packages/shared touched) |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — skipped per CI adaptation; no .cs changes in this diff anyway |
| Tests (dotnet) | N/A — same; PR body's own local proof (check-dashes, check-frontmatter, node --check, JSON parse, lefthook dashes/protect-main) is consistent with the diff |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8, 9, 10 (DESIGN.md/AI-slop, web↔mobile parity, i18n) — N/A, frontend-only / no
apps/*touched. - Dimension 11 (Contract drift + backward-compat) — N/A, no DTO/route/shared-type change; no candidates.
- Dimension 13 (Backend hard rules) — N/A, no
src/Orbit.*touched. - Dimension 14 (FEATURES.md parity) — N/A, no user-facing feature surface changed.
- PR body claim that
orbit-ui-mobilePR #659 landed the sibling rule — not independently verifiable, mobile repo not checked out in this job. - Whether
main's branch protection hasenforce_adminsenabled — the new "never admin-merge" bullet sits underAGENTS.md's pre-existing framing that the guardrail list is "enforced by ... GitHub branch protection," but an admin-merge override bypasses branch protection by definition unlessenforce_adminsis specifically on. Not confirmable from this diff alone. Not raised as a ranked finding (no evidence either way), but worth a human check: ifenforce_adminsisfalse, either enable it or soften the bullet's implied technical backstop to "policy-only."
What's good
- Fixes a real, previously-silent guard gap (PowerShell-issued git commands bypassed branch protection entirely).
- Rigorous self-verification in the PR body, matching the standard its own new rule demands of future changes.
- New
AGENTS.mdcontent is adapted to this repo's externals, not pasted verbatim, and correctly cross-numbered. - Correctly scoped: explicitly excludes items still blocked on the unlanded
orbit-ui-mobilepeer PR rather than landing a partial lockstep pair.
Recommendation
Approve. Optional non-blocking follow-up: confirm enforce_admins: true on main's branch protection so the new never-admin-merge bullet has the technical backstop its section's framing implies.
Branch protection on main requires three code scanning contexts: "Analyze
(csharp)", "Analyze (javascript-typescript)" and "Analyze (actions)". They
were produced by CodeQL default setup, which does not run on Dependabot pull
requests (github/codeql-action issue 2858). The three contexts therefore never
reported on a Dependabot head, and every Dependabot pull request sat
permanently BLOCKED waiting on checks that could not arrive.
Default setup is now disabled on the repository, and this workflow takes over.
An advanced-setup workflow is an ordinary workflow, so it triggers on a
Dependabot head like every other job here, and its job-level permissions block
raises the read-only Dependabot GITHUB_TOKEN to security-events: write.
The matrix job name "Analyze (${{ matrix.language }})" interpolates to exactly
the three required context names, so branch protection needs no change. C# uses
build-mode manual with the same dotnet restore plus dotnet build --no-restore
that API Tests already proves, because this repository generates code at build
time; javascript-typescript and actions use build-mode none. github/codeql-action
is pinned by SHA to tag v4.37.4, matching the repository convention for
non-actions/* actions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
There was a problem hiding this comment.
Code Review: PR #442
Scope: Diff since the last review (commit 988e04c1), which added .github/workflows/codeql.yml. Earlier commits (.claude/hooks/git-guardrails.mjs, .claude/settings.json, AGENTS.md) were already reviewed and approved in a prior review at commit c0175eae and are not re-flagged here.
Recommendation: APPROVE
Summary
This commit adds an 87-line CodeQL "advanced setup" workflow replacing GitHub's default CodeQL setup. The motivation — default setup doesn't run on Dependabot-authored PRs, permanently blocking the three required status-check contexts — is a real, known codeql-action limitation. The workflow reproduces the three required check names (Analyze (csharp), Analyze (javascript-typescript), Analyze (actions)) via matrix-job-name interpolation, uses least-privilege job-level permissions (security-events: write, actions: read, contents: read), SHA-pins github/codeql-action, and reuses this repo's proven dotnet restore && dotnet build --no-restore build steps from test.yml for the C# manual-build-mode leg. No src/ or product code is touched, so backend hard-rules, security-reviewer, and contract-aligner gates don't apply.
Findings
Critical / High
None.
Medium
None.
Low / Info
- The commit message asserts CodeQL default setup is now disabled on the repository — this is an external GitHub repo-setting change not visible in the diff, and not independently verifiable from this CI job (the
code-scanning/default-setupAPI check was blocked by sandbox permissions). If that toggle wasn't actually flipped, advanced-setup SARIF uploads would be rejected while default setup is still on. Not blocking — worth a quick human glance at Settings → Code security. actions/checkout@v7/actions/setup-dotnet@v6are tag-pinned rather than SHA-pinned in the new workflow, consistent with every other workflow in this repo.github/codeql-actionis correctly SHA-pinned, matching the repo's convention for third-party actions.- No stale references to CodeQL default setup remain elsewhere in the repo that would need updating.
What's good
- Correctly diagnoses and fixes a real, previously-blocking CI gap (Dependabot PRs stuck on default-setup contexts that never report).
- Job-name interpolation is explicitly called out as load-bearing for branch protection.
- Reuses this repo's own proven C# build steps rather than inventing new ones.
- Permissions scoped tightly at workflow and job level; SHA-pinning matches repo convention.
Validation
Build/Test: N/A — skipped per CI adaptation (separate required checks handle this); no .cs changes in this diff.
Deferred / not verifiable in CI
- Dimensions covering frontend/mobile parity, contract drift, and FEATURES.md: N/A — no product code, DTOs, or shared types touched.
- CodeQL default-setup-disabled claim: not independently verifiable from this job's
gh apiaccess; flagged as Info above.
"SonarCloud Code Analysis" is a required context on main, and the SonarQubeCloud app only posts it once an analysis is uploaded. The scanner job was skipped on Dependabot heads because SONAR_TOKEN did not exist in the repository's Dependabot secret store, so the required context never reported and every Dependabot pull request stayed BLOCKED. SONAR_TOKEN is now present in that store, so the guard has nothing left to protect and is removed. The job already reads the secret as secrets.SONAR_TOKEN, which is the reference a Dependabot-triggered run resolves against the Dependabot store. claude-review.yml identified Dependabot with github.actor, which is whoever triggered the run, not the pull request author. A human re-run of a Dependabot run would therefore let the Claude review step through with an empty CLAUDE_CODE_OAUTH_TOKEN, since that secret lives only in the Actions store. It now checks the author, matching dependabot-auto-merge.yml. That step stays skipped for Dependabot on purpose; "review" is not a required context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH
|
thomasluizon
added a commit
that referenced
this pull request
Sep 6, 2026
thomasluizon
added a commit
that referenced
this pull request
Sep 7, 2026
thomasluizon
added a commit
that referenced
this pull request
Sep 8, 2026
thomasluizon
added a commit
that referenced
this pull request
Sep 8, 2026
thomasluizon
added a commit
that referenced
this pull request
Sep 9, 2026
* Prepare atomic streak gap repair (#442) * Add atomic repair for contiguous streak gaps (#442) * Preserve freeze award history across streak gap repair (#442) * Protect legacy streak award cursor (#442) * Refresh architecture map (#442) * fix: make gap repair follow the schedule, not the calendar Streak continuity runs over SCHEDULED occurrences, never calendar days: a weekly habit's streak survives the six unscheduled days between two occurrences. Three places asserted calendar adjacency instead, so a weekly or every-N-day user could never repair a gap at all. The day before the gap is not a scheduled occurrence, so it carries neither a completion nor a freeze, and the check always refused. The rule moves to where the schedule is actually known: - UserStreakService walks the ordered scheduled occurrences. The selection must be an unbroken run of them, so a caller still cannot omit a missed occurrence inside the gap, and the PRECEDING SCHEDULED occurrence must be completed or frozen. - StreakFreeze.CreateGap drops calendar-consecutiveness. That entity cannot see a schedule, so it was rejecting valid gaps before one was ever loaded. It keeps what a date list alone can prove: real dates, no duplicates, and a last date of local yesterday. - RepairStreakGapCommandValidator likewise checks duplicates only. Red first, on the half that carries the defect. Reverting just the preceding occurrence lookup to `gapStart.AddDays(-1)` fails the new weekly case; restored, the suite is 23/23. The second new case keeps the widening honest: when the prior week's occurrence was ALSO missed, one selected date is not a whole gap and repair stays refused. Schedule-awareness widens which gaps are contiguous; it never waives the precondition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: carry the scheduled predecessor, and reach it at the yearly boundary Three defects from the same root as the previous commit: continuity is defined over scheduled occurrences, and three more places still assumed calendar days. 1. THE AWARD CURSOR. The handler passed `gap[0].UsedOnDate.AddDays(-1)` to RestoreStreakAfterGapRepair while the service had already computed the real scheduled predecessor. On a sparse cadence those differ, the cursor match in User.RestoreStreakAfterGapRepair failed, and the derived-cursor fallback marked a newly crossed seven-day milestone as awarded WITHOUT granting its freeze, permanently suppressing the reward. The predecessor now travels on UserStreakState instead of being re-derived a layer up, the same one-source shape the readiness fallback needed. 2. THE YEARLY BOUNDARY. A yearly occurrence on yesterday has its predecessor 366 days before the gap, but history loaded only MaxStreakLookbackDays = 365. The predecessor fell outside the window, so the gap was index 0 and every yearly and every-365-day gap was refused. The window widens by MaxScheduleSpanDays, the longest span between two consecutive occurrences of a supported cadence, so it stays bounded. The GAP itself is still confined to the ordinary streak window; only its predecessor may sit in the widened margin. 3. Three tests pinned the calendar rule the previous commit removed. Each is updated rather than deleted, because the behaviour they guard survives: a calendar-sparse selection is still refused and still spends no freeze. It is now refused by the schedule-aware evaluation rather than the domain factory, which is exactly the relocation that makes a real weekly or yearly gap repairable. Full orbit-api suite: 6150 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: union both schedule windows instead of widening one past its cap The previous commit widened the schedule request to reach a yearly predecessor and broke ordinary habits doing it. GetStreakScheduledDates caps a generated range at MaxRangeDays, so one 731-day request truncated the RECENT end: for a habit created before the widened start, the gap ending yesterday fell outside the generated dates and a valid repair reported unavailable. My yearly fixture hid it because that habit starts AT its predecessor, so effectiveFrom moved forward and the truncation never fired. Two bounded queries now, unioned. Each is inside the cap, so nothing is lost at either end. The yearly case then failed for a different reason, found by probing the helper rather than by reasoning about it: both queries returned exactly the right dates, but ComputeStreakAsOf still walked from the recent bound, so the predecessor that MAKES the repair worth anything sat outside the walk. The repaired streak scored no gain over the current one and the repair was refused as useless. The walk now starts at the earlier bound, which is where the schedule now starts. With the union in hand the predecessor is simply scheduled[gapStartIndex - 1] again, so the second lookup branch is gone. Regression added, per the required outcome: a daily habit created 800 days ago, older than the widened start, still repairs a gap ending yesterday. Full orbit-api suite: 6151 passed, 0 failed. One unrelated accountability test flaked once mid-session and passes 3/3 in isolation and in the two runs either side of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: decide eligibility over the window the streak engine actually uses Reverts the widened history window. Widening only the repair path made eligibility and representation disagree, and the review caught the consequence: - EvaluateGapRepairAsync accepts the repair on widened history and spends a freeze; - the handler builds its response through GetStreakInfoQuery, which calls CalculateStateAsync on the ORDINARY 365-day window; - that window sees yesterday's freeze without the preceding completion, so it computes a zero streak; - a later RecalculateAsync persists that zero, after the freeze was spent. The user pays a freeze and gets nothing. Refusing a repair is strictly better than accepting one that cannot survive the next recalculation. So: ONE window, the engine's own, and the invariant written down. Eligibility is decided over exactly the history that can represent the result. Weekly and every-N-day gaps, which is what this ticket asked for, work entirely inside it. Yearly gaps stay refused, exactly as they were before this PR, so nothing regressed. The test now pins WHY rather than asserting they work, and the fix belongs to the engine's window: filed as thomasluizon/orbit-tickets#483 with the two traps recorded, including that GetStreakScheduledDates caps a per-habit range at MaxRangeDays so any future widening must query in bounded slices. MaxScheduleSpanDays goes with the approach that needed it. Full orbit-api suite: 6151 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: state the CreateGap rationale as XML-doc, not narration The Roslyn ORBIT0001 gate caught my own violation: a five-line `//` block narrating why calendar-consecutiveness is absent. Only XML-doc or a WHY note linking an upstream URL is allowed. The rationale is worth keeping, so it becomes a summary plus remarks on the method rather than being deleted. Verified the way CI builds it: `dotnet build --no-incremental` is clean with zero ORBIT diagnostics. The earlier local `dotnet test` runs never recompiled this file against the analyzer, which is why CI saw it first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: keep the first break's cursor, and bound the legacy fallback Two award-cursor defects, both of which silently cost the user a freeze. 1. A LATER DECREASE OVERWROTE THE SNAPSHOT. SetStreakState captured the pre-gap cursor on EVERY decrease, so log, unlog and relog of today after missing yesterday replaced (7, Sep 4), which identifies the repairable gap, with (0, Sep 6), which describes the break itself. Repair's exact match then failed and the derived cursor skipped a milestone that was never granted. Only the FIRST decrease captures now, and the pair is captured together so the date can never belong to a different run than the streak beside it. 2. THE LEGACY FALLBACK ROUNDED THE WRONG STREAK. With no matching snapshot, which is every pre-migration row, the cursor came from the FULL repaired streak, so milestones crossed by completions AFTER the gap were recorded as already awarded and their freezes were never issued. A row with six completions before the gap and one after reached 7, spent its banked freeze, and could never receive the one it had just earned. The fallback is now bounded by the streak going INTO the gap, which the service computes as of the scheduled predecessor and carries on UserStreakState. The cursor cannot advance past what was earned before the gap, so a newly crossed milestone stays awardable. `RestoreStreakAfterGapRepair` takes preGapStreak as a required parameter rather than an optional one: every caller should have to say what was earned before the gap, and there is exactly one production caller. Full orbit-api suite: 6153 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * test: prove the cursor fixes where they can actually fail The review asked for these regressions to run through UserStreakService and the handler rather than mutating User directly. One of the two cannot honestly be written that way, and the first attempt at it was a test that could not fail. The legacy-fallback case IS now handler-level: six completions before the gap, one after, a pre-migration row with no snapshot and a banked freeze, driven through RepairStreakGapCommandHandler. It proves the milestone crossed after the gap stays awardable. The capture-once case stays on the entity, and the test says why. Driving it through RecalculateAsync needs an increase followed by a decrease, and this fixture cannot produce one: measured, Unlog leaves the recalculated streak at 1 rather than returning it to 0, so the service never sees a second decrease. The service-level version therefore passed with the fix REVERTED and proved nothing, which is worse than no test because its name claimed coverage it did not have. Red first, both, against the same revert: ASecondDecrease_KeepsTheSnapshotFromTheFirstBreak FAIL LegacyRowCrossingAMilestoneAfterTheGap_StillEarnsItsFreeze FAIL restored: 28 passed BuildRepairHandler is extracted now that a third caller exists, and the existing milestone theory uses it instead of building the same eight substitutes inline. Full orbit-api suite: 6153 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: grant the milestone repair earns, and cap a stale saved cursor Two more cursor defects, both mine. 1. REPAIR MADE A MILESTONE ELIGIBLE AND NOTHING GRANTED IT. Restoring the cursor only clears the way; the response is built by GetStreakInfoQuery, which calls CalculateAsync rather than RecalculateAsync, so an award earned by the repaired run sat pending and was lost the next time the streak reset. The repair now grants it in the same save that spends the bank. My earlier test hid this by calling AwardStreakFreezeIfEligible by hand, which production never does; it now asserts the freeze is actually banked. 2. THE SAVED CURSOR OUTRANKED THE RUN IT DESCRIBED. The snapshot deliberately survives later recalculations, so it can describe a LONGER run than the one being repaired: unlog an older pre-gap completion and a cursor of 14 still matches on date while the run is now 7, suppressing awards the shorter run has re-earned. The saved value is capped by what the current pre-gap run supports. And a hazard in my own design, caught by two existing tests going red: PreGapStreak defaulted to 0, so any caller that did not supply it read as "the run into the gap was zero", collapsing the cursor and re-granting milestones the user already had. It is nullable now. Unknown means DO NOT BOUND, which is the behaviour that stood before the bound existed, rather than silently meaning zero. Full orbit-api suite: 6154 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix: move the cursor rationale into XML doc for ORBIT0001 CI rejected an eight-line narration block in User.cs. The rationale is worth keeping, so it moves onto the preGapStreak parameter documentation, where it also reaches anyone reading the signature rather than only someone inside the method. A correction worth recording, because I got this wrong twice tonight: the ORBIT Roslyn analyzers DO NOT run in the local `dotnet build`. A clean local build, even `--no-incremental`, proves nothing about ORBIT0001. Only CI catches it, and only for the Orbit.Domain project, which is why identical `//` comments in Orbit.Application and Orbit.Infrastructure pass. Check Domain comments by eye or push and read CI; do not trust a green local build for this rule. Full orbit-api suite: 6154 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(gamification): use XML block comments so ORBIT0001 accepts them CI failed the Build on ten ORBIT0001 diagnostics in `Orbit.Application`, not in `Orbit.Domain`: seven in `RepairStreakGapCommand` and three in `RepairStreakGapCommandValidator`. The analyzer covers every project except migrations and generated files, so the project is not what made the difference. A clean local build proves nothing here, and now there is a measurement for why. `dotnet build` on this tree emits `warning CS9057`: the analyzer assembly references compiler version 5.9.0.0 while the local SDK runs 5.6.0.0, so Roslyn DISABLES the analyzer and the build reports 0 errors on the exact file CI then rejects. Locally the ORBIT rules do not run at all, in any project. `NoCommentsAnalyzer.IsAllowed` accepts a multi-line comment that starts with `/**`, so the three narration blocks keep every word and change only their form. Nothing was deleted: each one records a defect this pull request fixed, and a sweep of the whole diff for added `//` lines outside `///` now returns nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ * fix(streaks): finish the ORBIT0001 sweep across every analyzed project The previous commit fixed `Orbit.Application` and CI then failed on nineteen more diagnostics in `Orbit.Infrastructure`. The build stops at the first failing project, so fixing one only reveals the next, and my first sweep was truncated by a `head -30` and reported clean when it was not. This is the whole sweep, run to completion. `Directory.Build.props` attaches the analyzer to EVERY project except the analyzers themselves, tests included, so the pass covers `src/` and `tests/` together: five blocks in `UserStreakService` and four in `StreakGapRepairTests`. Grepping the complete repository for an own-line `//` outside `///`, a URL note, a migration and generated output now returns nothing. Every word is kept. `NoCommentsAnalyzer.IsAllowed` accepts a multi-line comment that starts `/**`, so only the form changed, and the converter refuses a block containing `*/` rather than silently ending the comment early. Local evidence and its limit: `dotnet build` reports 0 errors on this tree and proves nothing about ORBIT0001. Every project emits `warning CS9057`, because the analyzer assembly references compiler 5.9.0.0 while the local SDK runs 5.6.0.0, so Roslyn disables it. The rules do not run locally at all, in any project. Only CI can confirm this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ * fix(streaks): repair inside the habit lock, so eligibility cannot go stale Pullfrog raised this three times and was right every time. I resolved the first two claiming it was pre-existing and architectural. That premise is false, and two commands disprove it: git log --oneline origin/main -S "EvaluateGapRepairAsync" -- src/ no output git cat-file -e origin/main:.../RepairStreakGapCommand.cs absent Both the evaluation and the handler are introduced by THIS pull request, so the read-then-write hazard is created here and cannot be deferred to a ticket. The defect: EvaluateGapRepairAsync decides from the habits, the completion logs, the freezes and the schedule derived from them, and the save then spends the freeze bank. Those were two separate snapshots. `User.xmin` cannot hold them together, because a schedule-only habit edit commits WITHOUT touching the user row, so the optimistic token this command already retries on never fires. A cadence or due-date change landing between the two left a freeze spent on a date the committed schedule no longer accepts, with the bank charged all the same. The boundary already existed and this handler simply never joined it. HabitCeilingLock takes a transaction-scoped per-user advisory lock, and UpdateHabitCommand, LogHabitCommand, CreateHabitCommand, MoveHabitParentCommand and RestoreHabitCommand all hold it. The repair now runs inside HabitCeilingLock.ExecuteAsync, so evaluation and the spend happen in one transaction under the same key every writer whose output it reads must take. The user is loaded inside the lock too: a load taken before it would carry a pre-lock snapshot of the bank into a post-lock decision. The regression asserts ORDER rather than presence, because presence proves nothing about the window: lock, then evaluate, then save, with the key read from HabitCeilingLock.ForUser itself so a rename cannot leave it passing against a lock nobody else holds. Wiring the transaction exposed six failing end-to-end cases in StreakGapRepairTests: a substituted IUnitOfWork returns null from the transaction wrapper unless the operation is invoked. Both sites now run it as the real one does, which is also what makes those cases exercise the real boundary. Ticket 484 stays open, scope corrected, for what this does not cover: writing the invariant down, sweeping other handlers that spend a bank from an unprotected Habit or HabitLog read, and deciding whether those tables should carry a concurrency token so the hazard is detectable rather than only preventable. Test evidence: dotnet test, 6155 passed, 0 failed Orbit.Analyzers.Tests 32 passed Orbit.Domain.Tests 582 passed Orbit.Application.Tests 3313 passed Orbit.Infrastructure.Tests 2228 passed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ * fix(gamification): extend the habit consistency boundary to every writer (#484) Pullfrog was right that `HabitCeilingLock` did not deliver the boundary this pull request claimed. Counted live on `main`: `DeleteHabitCommand`, `SkipHabitCommand`, `BulkLogHabitsCommand` and `BulkSkipHabitsCommand` had ZERO references to it, and `LogHabitCommand` had one, on the unlog branch only. So a soft delete could commit between the repair's eligibility read and its spend, leaving a banked freeze paid for a schedule that no longer existed. Every persisted mutation of habit schedule, completion logs or freeze currency now commits inside the lock: - `DeleteHabitCommand`, `BulkDeleteHabitsCommand` - `SkipHabitCommand`, `BulkSkipHabitsCommand` - `LogHabitCommand`'s log path, `BulkLogHabitsCommand` - `RepairStreakCommand`, the other currency spender, which had the same read-then-write shape as the gap repair and none of its protection - the chat tools' single commit point in `ProcessUserChatCommand.Persistence`, which is where every Astra habit mutation actually lands - the offline-sync habit delete replay in `SyncController` A writer only holds the lock across the save. The lock is transaction scoped, so what it serializes is the interval from acquisition to commit; derived work that follows, such as streak recalculation, gamification, challenges and referrals, stays outside it and keeps writing the xmin-tokened user row. Two unique-violation catches moved OUT of their transactions. A failed statement aborts a PostgreSQL transaction block, so the reply query each one runs could not have executed inside it. `HabitCeilingLock` becomes public for the one caller outside `Orbit.Application`. The regression runs `DeleteHabitCommandHandler` concurrently with the repair against a serializing unit of work and asserts the interleaving, not a call count. Removing the lock from the delete alone turns it red in 10 seconds. The serializing fake moved out of `HabitCeilingLockTests` so both suites share one. Full suite green: 6,156 tests across analyzers, domain, application and infrastructure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Yk8nzfE1spBP4VbMo4o53 * fix(gamification): use doc comments so ORBIT0001 accepts the boundary notes The Roslyn analyzer bans `//` comments and it did not run locally: the build printed CS9057 and skipped `Orbit.Analyzers.dll`, because that assembly references a newer compiler than the installed SDK. So the ban only fired in CI. Every note added by the previous commit is now a `/** */` block, matching the file it sits in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Yk8nzfE1spBP4VbMo4o53 * chore(arch): refresh the generated map for the new boundary test Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Yk8nzfE1spBP4VbMo4o53 * fix(tests): finish the ORBIT0001 sweep in the concurrency regression The previous sweep covered src/ and missed the new test file, so the same analyzer that does not run locally failed the build again on three comments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Yk8nzfE1spBP4VbMo4o53 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Harness-Lockstep-Peer: 665
ORB-164, the orbit-api half of the harness repair specification carried by ORB-163. This pull request lands only the three items that do not depend on the paired orbit-ui-mobile pull request. The remaining items stay for a later pass, because each one is bound to an edit that has not landed in #665 yet: deleting
.github/workflows/claude-review.ymland the four twins that name it, thepr-review/rubric.mdtwin edit, the byte-exactsecond-opinion.mjscorrection, and thepr-review/SKILL.mdclause 6 and 7 corrections.What changed
1.
git-guardrails.mjsnow runs on the PowerShell tool as well as Bash..claude/settings.jsonregistered its onlyPreToolUsehook on the matcher"Bash", so the identical git command issued through the PowerShell tool fired no hook at all and this repository's local branch protection was silently absent there. There are now two matchers,BashandPowerShell, both invoking the same hook.2.
AGENTS.mdcarries the "Never assume an external interface" rule at line 48, with its review-rule counterpart as Code Review Rule 5 at line 97. orbit-ui-mobile PR #659 landed this rule in its ownAGENTS.md, but codex workers launched into an orbit-api worktree read THIS repository'sAGENTS.md, so the rule did not reach them. Adapted to this repository's externals (dotnet, NuGet) and its existing review-rule numbering rather than pasted.3.
AGENTS.mdcarries the never-admin-merge prohibition from specification section J3a, at line 33, alongside the existing branch-protection guardrail. It names all three shapes:gh pr merge --admin, a directPUT /repos/{owner}/{repo}/pulls/{number}/merge, and the GraphQLmergePullRequestmutation. Naming both raw API calls is load bearing, because forbidding only the CLI flag leaves both API paths open. The one legitimate exception is stated: if a merge genuinely needs the admin override, the agent stops and asks Thomas to merge it himself.The branch was also renamed from
thomasluizon/orb-164-harness-repair-api-halftofeature/orb-164-harness-repair-api-half, which is whatCLAUDE.mdrequires.Proof that the guard actually refuses
Observed live in this worktree against the final file state, through a headless session rooted at the worktree so this repository's
.claude/settings.jsonis the project settings in effect. Both attempts were refused before the command reached git.Through the PowerShell tool:
Through the Bash tool:
git-guardrails.mjs:24readsinput.tool_input.command. The refusal echoinggit push origin mainback verbatim is the confirmation that the PowerShell tool populates that exact field, so the hook needed no payload change. The hook exits 0 silently whencommandis not a string, so a missing field would have produced a silent pass rather than this block. Localmainandorigin/mainwere both at319c774cwhen the attempts ran, so a hook miss would have been a no-op push rather than a real one.Branch protection payload, recorded before the first edit
Specification section J2 requires this because no revert restores branch protection state. Read from
GET /repos/thomasluizon/orbit-api/branches/main/protectionon 2026-07-30,strict: true, 15 required contexts:This pull request changes no branch protection setting and moves no
guards.ymljob name.Harness Lockstep on this pull request, and why
The three files changed here are
AGENTS.md,.claude/settings.jsonand.claude/hooks/git-guardrails.mjs. None of them is one of the six lockstep twins thatorbit-ui-mobile/tools/check-lockstep.mjs:16-24fixes at exactly six entries, and none of them isguards.yml. TheDetect a lockstep changestep atguards.yml:77-87therefore evaluates tochanged=false, every following step is skipped, and the job reports success without resolving the peer at all. SoHarness Lockstephere is green because the twins were untouched, not because the pair is complete. It is not evidence that the twin edits are done, and the later pass that lands them will be the run that actually exercises the sibling checkout.The
Harness-Lockstep-Peer: 665directive is on the first line regardless, because orbit-ui-mobile#665's mirror job requires the exact reciprocal line naming this pull request's number in this body. #665 must carryHarness-Lockstep-Peer:with this pull request's number or its ownHarness Lockstepjob fails.Also fixed while in the file
git-guardrails.mjscarried two em dashes, one of them inside the message the hook prints to the user.tools/check-dashes.mjsskips.claude/entirely, so the repository wide dash ban never saw them. Both are gone.Testing
No product code and no new decision path in a tool, so no unit test.
node tools/check-dashes.mjs --files AGENTS.md .claude/settings.json .claude/hooks/git-guardrails.mjsexits 0,node tools/check-dashes.mjs --check-baselineexits 0,node tools/check-frontmatter.mjsreports 8 files parsing,node --check .claude/hooks/git-guardrails.mjsexits 0, and.claude/settings.jsonparses as JSON. The lefthookdashespre-commit step and theprotect-mainpre-push step both ran green.Linear: ORB-164
🤖 Generated with Claude Code
https://claude.ai/code/session_01Bojq5cLFZ87iBD8zhnoSWH