tools/agentconfig: the required-substring pin for `.claude/claude-security-guidance.md` does not lock the claim it was added to protect
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
TestHostedReviewGuidanceStaysBoundedAndSelfContained pins "deterministic" as the required substring for .claude/claude-security-guidance.md. That string matches the section heading ## Workflow-side code stays deterministic, so it proves the section exists and nothing about what the section says. The paragraph under it can be rewritten into something false and the test stays green.
That is not hypothetical. The paragraph was wrong in #1960's first two heads and took a dedicated exact-head review round to catch. It said:
The engine threads a deterministic clock;
time.Nowis the wrong one.
Both halves were false. The durable driver reads Temporal's workflow.Now rather than an engine-threaded clock, v1.Clock's own doc says "The durable driver does not use this type at all", and the clock the local driver reads has time.Now as its production implementation (pkg/flowstate/v1/clock.go: func (realClock) Now() time.Time { return time.Now() }). The reachable harm was specific: a reviewer following that sentence could bless a ClockFromContext(ctx).Now() read inside a workflow function — which pkg/flowstate/v1/engine/evalcontext.go makes compilable, since it hands workflow-side code a real context.Context — resolving to RealClock and producing exactly the replay nondeterminism invariant 4 forbids.
It was corrected in 4f0d6bb to name both drivers' time sources. The test did not catch the original error and would not catch a third rewrite that reintroduced it.
Why this is the pin's problem rather than the prose's
The required-substring mechanism is sound for rules whose wording is stable, which is what the REVIEW.md pins are ("at most five nits", "is not a new finding"). It is weak exactly where a claim is a statement of fact about the tree that a future editor might paraphrase. A heading is the worst possible anchor for that, because a heading survives any rewrite of what sits beneath it.
Acceptance
Pin the corrected claim rather than its heading: add workflow.Now and time.Now to the required list for .claude/claude-security-guidance.md, so a rewrite that drops either time source fails. Both strings are load-bearing in the current text and neither is incidental.
Consider the same question for the other pins on that file. "belongs to nobody and is refused" and "does not bound the walk that produced" are claim text and hold up; "deterministic" and "textContent" are single words, and "textContent" is at least the exact API the claim is about rather than a heading.
Worth deciding whether a claim-shaped assertion belongs in this test at all, or whether a guidance file that makes checkable statements about the tree should cite them the way docs/ does and be covered by tools/citations. #1960 deliberately refused line-numbered citations in these two files, on the grounds that instructions to a reviewer have no reason to cite a line and nothing checks them — that reasoning still holds for line numbers, but a symbol-level check would not have the drift problem.
Provenance
Optional, non-blocking finding from the flowstate-reviewer exact-head review of #1960 at 4f0d6bbdbdac51404022f9e1e62f96aa441a7d5d, which returned PASS. Deferred here per .agents/ship.md rather than producing a fourth head. Two smaller wording notes from the same review are not worth their own issue and are recorded here in case that paragraph is edited again: "an injected Clock" is slightly loose for production, which defaults to RealClock rather than injecting, and "a Clock read there" leans on the trailing "not part of the durable path" clause for its scope where the word "durable" would carry it directly.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at TestHostedReviewGuidanceStaysBoundedAndSelfContained and inspect the required substrings for .claude/claude-security-guidance.md. Add workflow.Now and time.Now to that list, then run the focused test. Done means the test fails when either load-bearing time source is removed from the guidance text.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- testing-qa
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 88/100